Repository navigation
[BRK] Methods with inplace parameter should return None instead of Self #1266
Description
Description
Triangle methods that have an inplace parameter tend to return Self, such as:
chainladder-python/chainladder/core/triangle.py
Lines 893 to 900 in e03d708
This is inconsistent with Pandas behavior, where such methods mutate the object in-place and return None.
For example, see the docs page on DataFrame.sort_values().
Changing the return value to None would also align with the behavior of built-in Python methods, such as list.sort(). See this discussion from Guido from way back in the day.
His justification for the practice is that it enhances readability and lets the user know what object chained methods are acting upon.
Is your feature request aligned with the scope of the package?
- Yes, absolutely!
- No, but it's still worth discussing.
- N/A (this request is not a codebase enhancement).
Describe the solution you'd like, or your current workaround.
methods with related bugs that require additional fix
Triangle.dev_to_val()[BUG] dev_to_val, val_to_dev, and _val_dev inplace argument not working #1477Triangle.val_to_dev()[BUG] dev_to_val, val_to_dev, and _val_dev inplace argument not working #1477Triangle._val_dev()[BUG] dev_to_val, val_to_dev, and _val_dev inplace argument not working #1477Triangle.cum_to_incr()[BUG]cum_to_incrdoes not respectinplace#1481
Deprecation warning already done for these methods, behavior change will occur in 2.0:
TrianglePandas.astype()Common.set_backend()Triangle.set_index()Triangle.incr_to_cum()Triangle.grain()TrianlgePandas.fillna()TrianglePandas.fillzero()
We should first:
1. Issue a warning for the 0.11.0 release that the return value for these methods will change to None.
2. After the 0.11.0, update the methods to return None.
Do you have any additional supporting notes?
See related discussion.
Would you be willing to contribute this ticket?
- Yes, absolutely!
- Yes, but I would like some help.
- No.
Activity
henrydingliu commentedon Sep 4, 2026
I'd like to push all breaking changes past 1.0.0.
genedan commentedon Sep 4, 2026
Should this one be scheduled for 2.0.0 then?
henrydingliu commentedon Sep 4, 2026
yes please. we can put in a deprecation warning right now though.
henrydingliu commentedon Oct 3, 2026
@genedan @priyam0k @kennethshsu we punted on the removal timeline in #1409. before i start working on the deprecation tag for the rest of these, can we please align on a removal timeline? i'm open to anything between 1.1 and 2.0
genedan commentedon Oct 3, 2026
I'm still confused as to why a lot of the breaking changes we identified prior to 1.0.0 aren't scheduled for it. Isn't the major version bump an opportunity to have them done then? And then the breaking changes identified after 1.0.0 get implemented in 2.0.0?
henrydingliu commentedon Oct 3, 2026
because we are close to 1.0.
but i suppose "when/what is 1.0" is still an open question.
genedan commentedon Oct 4, 2026
For this particular ticket, 2.0.0 then, that would be consistent with semver.
priyam0k commentedon Oct 4, 2026
Targeting 2.0 for the actual removal and return value switch aligns directly with semver, since altering inplace return semantics from Self to None or dropping parameters breaks existing chained calls. Issuing the FutureWarnings now in 1.0 gives users a clean migration window without introducing breaking behavior in minor 1.x releases.
henrydingliu commentedon Oct 4, 2026
I have a working decorator for this deprecation in #1468. but there's a big problem. we don't have a way to turn off the warning if the user is correctly not assigning .inplace(True) to anything. it will warn every time someone calls inplace=True. even if we factor all of it out of the codebase, it can still be annoying to users.
do you want to live with the annoyance or just stick a deprecation warning in the docstring?
genedan commentedon Oct 4, 2026
I think this one of the reasons why Pandas (and maybe other libraries) don't do the deprecation when fixing bugs. Sometimes you can't tell if someone was trying to use a feature the right way or the wrong way and the warning would be annoying to a user who was using it the right way. I think we run into the same problem with #1395.
Docstring warning is fine with me. I'm also okay with just fixing this one on the spot.
henrydingliu commentedon Oct 5, 2026
i had to re-familiarize myself with pandas' breaking changes around
astype(str)on NaN. It looks like Pandas announced the intention via a PDEP in 2024, put out a migration guide in 2025, then actually changed the code in pandas 3 in 2026. i think this is a reasonable approach that we can learn from.I know we've talked about a CDEP before. Honestly I'm not sure how many people would read it (I certainly don't read PDEPs even though I use pandas daily). I do think a highly visible migration guide ahead of 1.0 would do a lot of good. so it's up on #1472. with this increased visibility, i think we can afford to accelerate some of the deprecations.
i like this framing. what breaks this package when someone assumes that it works the same as pandas?
inplace=true(supplied object is mutated)tri == otherbreaks since it returns a single boolthe other two (inplace=True returns mutated object and passing slice into at) doesn't break if user just follows existing pandas behavior.
does it work for you if we accelerate the first two but still punt on the other two? the idea is that by 1.0, pandas behavior will work in this package (along with some non-pandasian behavior).
genedan commentedon Oct 8, 2026
I don't think users would need to read PDEP or CDEP, but once contributors wind up in our shoes they might. I didn't read them either until I got pretty deep into package writing (this year).
Works for me.
cum_to_incrdoes not respectinplace#1481