[FIX] raise when the fitted pattern cannot be applied to the triangle passed in - #1310
[FIX] raise when the fitted pattern cannot be applied to the triangle passed in#1310ppcvote wants to merge 1 commit into
Conversation
Pyright Type CompletenessView the full Project (full
Other symbols referenced but not exported by
Symbols without documentation:
Patch (exported symbols added or changed by this PR): 0.0% fully typed (0 / 8)
Patch symbol details
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1310 +/- ##
==========================================
+ Coverage 91.70% 91.73% +0.02%
==========================================
Files 96 96
Lines 5464 5481 +17
Branches 704 709 +5
==========================================
+ Hits 5011 5028 +17
Misses 328 328
Partials 125 125
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
0539256 to
fcbdf4b
Compare
|
thanks for the great PR! i have 3 very differently scoped comments
|
fcbdf4b to
83f0ef8
Compare
|
All three done or answered. I also refreshed the description above, which still carried the counts from before the rebase and the column check. ruff. Holding until the cleanup lands. Messages. Reworded, no mechanism named: Columns. Added here rather than deferred, because the existing suite turned out to be unaffected by it. The case is worse than the index ones. Without this PR's check: paid = clrd["CumPaidLoss"].groupby("LOB").sum()
incurred = clrd["IncurLoss"].groupby("LOB").sum()
cl.Chainladder().fit(cl.Development().fit_transform(paid)).predict(incurred)returned an ultimate of 228,088,946 labelled What I can say for it is that the 1142 tests on The check is one direction, Branch as it stands: 1156 passed, 7 skipped. |
… passed in intersection() narrows two triangles to their shared index, which is the right thing for the grain broadcast casact#400 asked for and the wrong thing for a mismatch. Four mismatches were resolving quietly: X carries a group the model was never fit on. Those rows are dropped, so Chainladder returned 643 of the 775 rows it was handed, with no warning, while BF and CapeCod raised a numpy shape error naming no index value. ldf_ is at a finer grain than X. Neither side is narrowed, predict returns an object whose triangle and ldf_ disagree, and Chainladder built a 775 row ultimate_ from a 6 row input. Both sides are a single row, which intersection short circuits on, so a pattern fit on one group was applied to another and labelled as the group predicted on. X carries a column the model was never fit on. A paid pattern applied to an incurred triangle came back labelled incurred, 228,088,946 against its own 150,105,776 on clrd. validate_ldf checks these, in the shape validate_weight already uses. It runs before the `X_new + (self.X_ ... * 0)` line rather than next to intersection, because that addition borrows self.X_'s index when both sides are a single row, which erases the identity the check needs to see. A test holds that ordering. A pattern whose index is entirely the "(All)" sentinel is exempt from the index checks: it carries no group identity, and test_different_backends relies on that. sum() stamps "(All)" on whatever subset it was called on, so this exempts a pattern summed from one line of business as readily as one summed from everything. Separating those needs aggregation provenance on the Triangle. A pattern coarser than X stays allowed, which is the casact#400 flow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
83f0ef8 to
956b847
Compare
|
clean-up landed!
i wasn't actually thinking about the single column case. if we were to check column value for single column triangle, it would be somewhat contrary to our approach on single index, no? let's lay out the different scenarios and align on the proper treatment
|
|
Rebased onto the cleanup. It had landed and touched all three files this PR does, so the branch went into conflict; separately, CI never ran on my previous push, which I cannot explain beyond noting it.
The one conflict was |
Summary of Changes
intersection()narrows two triangles to their shared index. That is the right thing for the grain broadcast #400 asked for, and the wrong thing for a mismatch, and it cannot tell the two apart. Three mismatches were resolving quietly:Xcarries a group the model was never fit on.intersectiondrops those rows, soChainladderreturned 643 of the 775 rows it was handed with no warning, whileBornhuetterFergusonandCapeCodraisedoperands could not be broadcast together with shapes (775,1,10,1) (643,1,10,1), which names no index value.ldf_is at a finer grain thanX.intersectionnarrows neither side, sopredictreturns an object whose triangle sits at the caller's grain and whoseldf_sits at the model's, andChainladderbuilt a 775 rowultimate_from a 6 row input.intersectionshort circuits on that at its first line, so a pattern fit on one group was applied to another and the result labelled as the group predicted on. Fitting on one company in comauto and predicting on othliab returns an ultimate of 3,309,931.25 carrying[['Aegis Grp', 'comauto']]as its index.validate_ldfchecks all three, in the shapevalidate_weightalready uses.Two things about it are worth flagging, because neither is obvious from the diff.
Where the call sits. It runs before
X_new = X_new + (self.X_.val_to_dev().iloc[0,0].sum(2) * 0), not next tointersection. That addition goes through_prep_index, which borrowsself.X_'s index when both sides are a single row, so by the time control reachesintersectionthe caller's identity is already gone and the check sees two triangles that agree. Placing the call after that line looks natural and silently does nothing for the single-row case.The
"(All)"exemption. A pattern whose index is entirely the"(All)"sentinel came fromTriangle.sumand carries no group identity, so neither its index values nor its key labels constrain what it may be applied to, and it is exempt from both checks. Without that, this rejectstest_different_backends, which fits onclrd.sum()and predicts on 132 companies. Row count cannot separate that from the single-group mistake above: both are one row sharing no index value with the target. The limit of that exemption is worth stating plainly, because it is wider than it first looks.Triangle.sumstamps"(All)"on whatever subset it was called on, so a pattern summed from a single line of business is exempt exactly as a pattern summed from the whole book is. Fitting ontri[tri.index["LOB"] == "wkcomp"].sum()and predicting on comauto is allowed and returns 157 rows, which is the third case above reached with one extra.sum(). Closing that means recording aggregation provenance on the Triangle, which is a larger change than this and your call rather than mine. What this PR does is remove the cases where nothing was summed at all.A pattern coarser than
Xstays allowed, which is the #400 flow.Columns, added after review. The same mismatch on the columns axis: fitting on
CumPaidLossand predicting onIncurLossreturned an ultimate of 228,088,946 labelledIncurLoss, where fitting on the incurred triangle gives 150,105,776.paid -> [paid, incurred]was allowed too. The check is one direction,set(X.columns) - set(ldf.columns); the reverse is noted on the thread.Related GitHub Issue(s)
Fixes #1288.
intersectionitself is untouched, per your point on that thread about #1037.Note for merge order: #1306 touches the adjacent lines in
CapeCod.predict, so whichever lands second will want a rebase.Additional Context for Reviewers
test_misaligned_index2. Those two cover predicting on a strict subset of the fitted data, which is the case this check is most likely to reject by mistake, so the boundary sits in one place. Removingvalidate_ldffrom both call sites fails exactly the five rejection tests; the two that assert a flow still works pass either way, which is what they are for.test_predict_checks_before_the_index_is_borrowedfails on its own if the call is moved back after the addition, so the ordering above is held by a test rather than by a comment.pytest chainladderpasses: 1156 passed, 7 skipped, against 1142 and 7 onmain. The fourteen are the seven new tests across both backend parametrisations; no existing test changes behaviour.ruff check --force-exclude --config lint.per-file-ignores={}reports the same 15 findings on these three files before and after, andruff format --checkreports the same hunks line for line.CapeCodcallsvalidate_ldfas well. For the finer-ldf_case that call is the only thing between the caller and a raw numpy error inside_get_capecod_aprioris, which runs beforeMethodBase.predictis reached. For the unseen-group caseMethodBase.predictwould catch it anyway throughsuper().predict, just after CapeCod has done work it did not need to.validate_weightnext door uses a multi-line signature and'''docstrings, andruff formatwants neither. I wrotevalidate_ldfthe way the formatter wants so it adds nothing to the format diff, rather than matching its neighbour.Declarations
I am adhering to the standards in the Governing Doc. I am a human contributor, not a bot, and I opened this because @henrydingliu asked for it on #1288.
AI disclosure, per the AI Usage Policy: I used Claude Code on this. It ran the reproductions across the estimators and all three directions, and found both the single-row case and the call-site ordering by running the suite against earlier attempts that were wrong in each of those ways. I reviewed the diff and the tests myself, and the suite was run locally on my machine.