correct n_periods logic to respect dropped link ratios

[andrewwilson201]

Resolves #604

This pull request addresses the issue where n_periods was applied before drop logic, leading to an incorrect number of periods being used in LDF calculations.

Changes:

• Modified Development.fit() to apply n_periods after considering drops
• Added _apply_n_periods_with_drops() helper method
• n_periods now counts only valid (non-dropped) periods when used with drop/drop_valuation
• Added unit tests for the new behavior
• Updated docstrings to document the improved functionality

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.96%. Comparing base (5309728) to head (867d9f5).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #605      +/-   ##
==========================================
+ Coverage   81.91%   81.96%   +0.04%     
==========================================
  Files          83       83              
  Lines        4805     4818      +13     
  Branches      606      606              
==========================================
+ Hits         3936     3949      +13     
  Misses        654      654              
  Partials      215      215              

Flag	Coverage Δ
unittests	81.96% <100.00%> (+0.04%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[henrydingliu]

See discussion here.

Will wait for issue closure prior to closing PR

[henrydingliu]

@jbogaardt How did this PR pass this test? Is it a pure coincidence?

[kennethshsu]

@jbogaardt How did this PR pass this test? Is it a pure coincidence?

This test doesn't test the scenario that @andrewwilson201 described. If you move n_periods down, it fails:

import chainladder as cl
import numpy as np

clrd = cl.load_sample("clrd")
clrd = clrd.groupby("LOB")[["IncurLoss", "CumPaidLoss"]].sum()
dev1 = cl.Development(
    n_periods=7,
    drop_valuation=1995,
    drop=("1992", 12),
    drop_above=1.05,
    drop_below=0.95,
    drop_high=1,
    drop_low=1,
).fit(clrd)
pipe = cl.Pipeline(
    steps=[
        # ("n_periods", cl.Development(n_periods=7)), # old n_periods location
        ("drop_valuation", cl.Development(drop_valuation=1995)),
        ("drop", cl.Development(drop=("1992", 12))),
        ("drop_abovebelow", cl.Development(drop_above=1.05, drop_below=0.95)),
        ("drop_hilo", cl.Development(drop_high=1, drop_low=1)),
        ("n_periods", cl.Development(n_periods=7)), # new n_periods location
    ]
)
dev2 = pipe.fit(X=clrd)
assert np.array_equal(
    dev1.w_v2_.values, dev2.named_steps.drop_hilo.w_v2_.values, True
)

[henrydingliu]

In this PR, doing Development(n_periods = 7, drop =("1992",12)) is supposed drop 1992 then go back another year. Which is different from pipeline(dev(n_periods), dev(drop)). So it's supposed to test the scenario that Andrew described, but it's somehow not failing

[kennethshsu]

Ahh I see your point, thank you. It's very possible there's another bug hidden in there.

[henrydingliu]

I tried to run the test manually in this branch. If my understanding is correct, the updated drop logic should give a 1 weight to 4 accident years in the first column, skipping 1995, but using 1996, 1994, 1993, and 1992? Here I print out a line from Development.fit to show that I'm indeed on the PR branch.

@andrewwilson201 Do you agree that it's not doing what it's supposed to be doing?