Repository navigation
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): 17.4% fully typed (4 / 23)
Patch symbol details
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1473 +/- ##
==========================================
+ Coverage 94.14% 94.56% +0.41%
==========================================
Files 96 96
Lines 5843 6439 +596
Branches 739 892 +153
==========================================
+ Hits 5501 6089 +588
- Misses 221 222 +1
- Partials 121 128 +7
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:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
afed399 to
7e7af1f
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7e7af1f. Configure here.
priyam0k
changed the title
henrydingliu
left a comment
henrydingliu
left a comment
There was a problem hiding this comment.
the scope creeping suggestions are optional. feel free to push back and we can leave them to future PRs.
| Parameters | ||
| ---------- | ||
| paid_to_incurred: tuple or list of tuples | ||
| paid_to_incurred: tuple or list of tuples, optional |
There was a problem hiding this comment.
borderline out of scope. but, if we are adding "optional" to the docstring, we should describe what the default values are. I'm also not sure if this argument is actually optional
There was a problem hiding this comment.
Removed , optional to restore exact Approach 1 signature.
| groupby=None, | ||
| **kwargs, | ||
| ): | ||
| if ( |
There was a problem hiding this comment.
do we need this if we never shipped impliedcase inside caseoutstanding?
There was a problem hiding this comment.
Removed **kwargs and unreleased backward-compatibility error guard.
| ) | ||
|
|
||
| # 1. Determine case triangle | ||
| if len(X.columns) > 1 and "incurred" in X.columns and "paid" in X.columns: |
There was a problem hiding this comment.
Since the estimator only makes use of a single column of X, I would argue that the sklearn convention is to only expect X to already contain only case reserves, and leave the preprocessing outside of the estimator.
There was a problem hiding this comment.
fit and transform now expect a 1-channel case triangle directly per scikit-learn standard, raising ValueError on multi-channel triangles.
| self.reported_pattern = reported_pattern | ||
| self.paid_pattern = paid_pattern | ||
| self.style = style | ||
| self.groupby = groupby |
There was a problem hiding this comment.
Removed unused groupby parameter from ImpliedCaseDevelopment.__init__.
| return X_new | ||
|
|
||
| @staticmethod | ||
| def _extract_cdf_series(pattern, style="cdf"): |
There was a problem hiding this comment.
this is a great method. however, i would recommend that it outputs a triangle directly.
|
|
||
| Attributes | ||
| ---------- | ||
| ldf_ : Triangle |
There was a problem hiding this comment.
i don't think the ldfs are meaningful in any practical way. certainly it doesn't predict calendar period emergence. i would recommend that we only retain case_cdf_ as the estimated attribute
There was a problem hiding this comment.
Documented case_cdf_ as primary estimated attribute; retained ldf_ internally for downstream Chainladder compatibility.
| self, | ||
| reported_pattern=None, | ||
| paid_pattern=None, | ||
| style="cdf", |
There was a problem hiding this comment.
could you please add type hinting?
There was a problem hiding this comment.
Added type hints across __init__, fit, transform, and helper methods.
priyam0k
left a comment
priyam0k
left a comment
There was a problem hiding this comment.
Addressed the review feedback: Left the DevelopmentConstant pattern generalization for a dedicated follow-up issue.
| groupby=None, | ||
| **kwargs, | ||
| ): | ||
| if ( |
There was a problem hiding this comment.
Removed **kwargs and unreleased backward-compatibility error guard.
| ) | ||
|
|
||
| # 1. Determine case triangle | ||
| if len(X.columns) > 1 and "incurred" in X.columns and "paid" in X.columns: |
There was a problem hiding this comment.
fit and transform now expect a 1-channel case triangle directly per scikit-learn standard, raising ValueError on multi-channel triangles.
| Parameters | ||
| ---------- | ||
| paid_to_incurred: tuple or list of tuples | ||
| paid_to_incurred: tuple or list of tuples, optional |
There was a problem hiding this comment.
Removed , optional to restore exact Approach 1 signature.
| self.reported_pattern = reported_pattern | ||
| self.paid_pattern = paid_pattern | ||
| self.style = style | ||
| self.groupby = groupby |
There was a problem hiding this comment.
Removed unused groupby parameter from ImpliedCaseDevelopment.__init__.
| self, | ||
| reported_pattern=None, | ||
| paid_pattern=None, | ||
| style="cdf", |
There was a problem hiding this comment.
Added type hints across __init__, fit, transform, and helper methods.
|
|
||
| Attributes | ||
| ---------- | ||
| ldf_ : Triangle |
There was a problem hiding this comment.
Documented case_cdf_ as primary estimated attribute; retained ldf_ internally for downstream Chainladder compatibility.
|
@priyam0k thanks for the revisions. in the future, could you please also click the "re-request review" so I can come back and approve? 
i also have a slightly out of scope question that I'm going to raise in the original issue. |
Sure, will follow from future. I have re-request for a review |
henrydingliu
left a comment
henrydingliu
left a comment
There was a problem hiding this comment.
a couple of minor comments
| self, | ||
| reported_pattern: Any = None, | ||
| paid_pattern: Any = None, | ||
| style: str = "cdf", |
There was a problem hiding this comment.
could you please update this to Literal["ldf", "cdf"]?
| "ImpliedCaseDevelopment requires both reported_pattern and paid_pattern." | ||
| ) | ||
|
|
||
| if len(X.columns) > 1: |
There was a problem hiding this comment.
i think this is fine for now. hypothetically, someone could have loss case reserve and alae case reserve as two columns, or net loss case reserves and ceded case reserves as two columns. but to meet a realistic multi-column use case, we'd also have to allow the user to pass in multi-column paid and incurred patterns. today, to do that on DevelopmentConstant is a right pain through callables. once we sufficiently enhance DevelopmentConstant, we can revisit the single column restriction
leaving this comment as evidence that we thought about it and intentionally shipped it this way
| self.ldf_ = dev_const.ldf_.set_backend(backend) | ||
| self.case_ldf_ = dev_const.ldf_.set_backend(backend) | ||
| self.case_cdf_ = dev_const.cdf_.set_backend(backend) | ||
| self.case_ = case_tri |
There was a problem hiding this comment.
is it okay that case_tri is not a copy of X but X itself?
2 participants
Summary of Changes
Implements Approach 2 of the Case Outstanding method from Friedland (2010, Chapter 12, Exhibit III):
Related GitHub Issue(s)
Closes #282
AI/LLM Usage
Used basic model for drafting initial unit test cases.
Additional Context for Reviewers
Follows discussion in #282 with @henrydingliu and @kennethshsu decoupling Approach 2 into a standalone estimator, analogous to Development vs DevelopmentConstant.
Submitter's Checklist
[FIX],[FEAT],[DOCS],[TST],[CHORE], or[BRK]).Note
Medium Risk
New actuarial development logic affects reserve projection paths; behavior is guarded by input validation and Friedland reconciliation tests but still warrants actuarial review.
Overview
Adds
ImpliedCaseDevelopment(andImpliedCasealias) as a standalone development estimator for Friedland “Approach 2”: implied case CDFs/LDFs are computed from benchmark reported and paid patterns (CDF or LDF viastyle), then wired throughDevelopmentConstantsoChainladderand similar estimators can consumeldf_on a single-channel case triangle.CaseOutstandingremains the empirical triangle-based method (Approach 1); its docs now point to the new class for benchmark-pattern workflows, with small doc/param clarifications (e.g.groupby).Exports and API docs are updated; tests cover Exhibit III numerics, pattern input types, validation, transform/
Chainladderintegration, and that Approach 2 kwargs are not accepted onCaseOutstanding.Reviewed by Cursor Bugbot for commit 8221ddd. Bugbot is set up for automated code reviews on this repo. Configure here.