Refactor Chapter 7 exhibit construction - #1226
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fc5f4f6. Configure here.
| " )\n", | ||
| " )\n", | ||
| "\n", | ||
| " return (results_upper, results_lower)" |
There was a problem hiding this comment.
Backup function left in notebook
Medium Severity
ex3sht1_BAK looks like a leftover backup of the pre-refactor Sheet 1 builder. It is never called; only ex3sht1 is used. Keeping both versions clutters the notebook and makes it unclear which implementation is canonical after the model_diagnostics rewrite.
Reviewed by Cursor Bugbot for commit fc5f4f6. Configure here.
There was a problem hiding this comment.
I am leaving this in for now as the reviewers have not determined what version of the function should be kept at this point.
| " tri: cl.Triangle,\n", | ||
| " dev_input: dict,\n", | ||
| " tail_input: dict\n", | ||
| ") -> tuple:\n", |
There was a problem hiding this comment.
Unused tail input parameter
Low Severity
ex3sht1 still accepts tail_input, and the caller still builds and passes it, but the new body never uses that argument. The old path applied TailConstant; the refactor dropped that without cleaning the parameter or call site, leaving dead API surface.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit fc5f4f6. Configure here.
There was a problem hiding this comment.
I will remove this once bigger issues have been resolved.
|
@henrydingliu, I used The issue with using I just found the original approach of merging individual pandas series in a dictionary to be cleaner, but I guess it is a subjective thing. @priyam0k , I bring this to your attention as well for input. |
|
@salexanian can you please rename this alternative solution as part_2a to circumvent the conflict? then we can run the test and look at the RTD render. |
|
No problem, @henrydingliu . I will do so. I'll be back home in a couple of hours |
@henrydingliu - done. |
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): no exported symbol type-completeness changes detected. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1226 +/- ##
==========================================
+ Coverage 90.86% 91.31% +0.44%
==========================================
Files 91 91
Lines 5331 5434 +103
Branches 677 700 +23
==========================================
+ Hits 4844 4962 +118
+ Misses 347 336 -11
+ Partials 140 136 -4
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:
|
|
I'm not sure why you are implementing an entirely new pattern of pandas manipulation. take summing, the new add_totals private method is less elegant than the previous approach. but why do add_totals in the first place? things like ultimate and ibnr are also directly available in model_diagnostic. i recommended model_diagnostic because it directly provides an output in pandas for further manipulation, shortcuttinf all the col1, col2, etc construction in the previous ex3sht1. apologies if it sounded like I wanted you to rewrite the whole method. |
No problem, @henrydingliu - I will adapt my existing function to obtain the values from that It was actually good to create the function above as it gave me valuable practice with Chainladder so it was time well-spent anyway from my perspective. |
2 participants
Summary of Changes
Follow-up to the merged Chapter 7 reconciliation work.
Refactors the Chapter 7 exhibit construction, including use of
cl.model_diagnostics()to reduce pandas wrangling and simplify the exhibit-building logic.The reconciliation assertions remain in place to verify the reproduced Friedland values.
Related GitHub Issue(s)
#1189
Additional Context for Reviewers
Have rewritten Ex3Sht1 to use cl.model_diagnostics.
Checklist
uv run pytest) and documentation changes (uv run --directory docs jb build . --builder=custom --custom-builder=doctest)Note
Low Risk
Documentation/notebook-only refactor of exhibit construction; no library API or calculation-path changes. Reconciliation checks against Friedland remain in place.
Overview
Follow-up to Chapter 7 reconciliation: refactors exhibit construction so Friedland Exhibit III Sheet 1 (
Ex3Sht1) is built fromcl.model_diagnostics()instead of ad-hoc pandas wrangling.Chainladder fit results (latest, ultimate, IBNR, etc.) are pulled from diagnostics and assembled into the scenario tables. Existing reconciliation assertions against Friedland values are unchanged.
Reviewed by Cursor Bugbot for commit 74ee07f. Bugbot is set up for automated code reviews on this repo. Configure here.