Skip to content

Refactor Chapter 7 exhibit construction - #1226

Open
salexanian wants to merge 7 commits into
casact:mainfrom
salexanian:friedland-ch7-refactor
Open

Refactor Chapter 7 exhibit construction#1226
salexanian wants to merge 7 commits into
casact:mainfrom
salexanian:friedland-ch7-refactor

Conversation

@salexanian

@salexanian salexanian commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

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

  • I passed tests locally for both code (uv run pytest) and documentation changes (uv run --directory docs jb build . --builder=custom --custom-builder=doctest)

Note

Low Risk
Documentation and doctest refactors in the Friedland chapter only; no production reserving or API behavior changes.

Overview
Refactors Chapter 7 Friedland exhibit construction (follow-up to reconciliation work for #1189), centering on Exhibit I, Sheet 3 (Ex3Sht1).

development_summary-style exhibit tables are rebuilt to pull Latest, CDF, and Ultimate (and related columns) via cl.model_diagnostics() instead of hand-rolled pandas indexing, transposes, and separate Chainladder().fit calls. Rounded exhibit paths that match the textbook’s three-decimal CDF workflow should stay aligned with the prior narrative.

Reconciliation assertions against the printed Friedland figures are kept so doctests still verify reproduced values after the simplification.

Reviewed by Cursor Bugbot for commit f84014d. Bugbot is set up for automated code reviews on this repo. Configure here.

@salexanian
salexanian marked this pull request as draft August 21, 2026 16:56
" )\n",
" )\n",
"\n",
" return (results_upper, results_lower)"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit fc5f4f6. Configure here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am leaving this in for now as the reviewers have not determined what version of the function should be kept at this point.

Comment thread docs/friedland/chapter_7_part_2a.ipynb Outdated
" tri: cl.Triangle,\n",
" dev_input: dict,\n",
" tail_input: dict\n",
") -> tuple:\n",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit fc5f4f6. Configure here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remove this once bigger issues have been resolved.

@salexanian

Copy link
Copy Markdown
Contributor Author

@henrydingliu, I used cl.model_diagnostics to create a new version of ex3sht1 that displays the tables correctly and passes the asserts, but the function is actually longer than the one I had produced originally. Could you please review to ensure I implemented it the way you expected?

The issue with using cl.model_diagnostics is that it essentially requires me to take the multi-index table returned by it, convert it to a single-index table, and then reconvert it to multiindex again - this ends up taking a reasonable amount of code to do. It may be that Exhibit 3 Sheet 1 is not an ideal exhibit to use with cl.model_diagnostics, and other exhibits will work more seamlessly with it. I also don't rule out the possibility that I implemented this incorrectly.

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.

@henrydingliu

Copy link
Copy Markdown
Member

@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.

@salexanian

Copy link
Copy Markdown
Contributor Author

No problem, @henrydingliu . I will do so. I'll be back home in a couple of hours

@salexanian

Copy link
Copy Markdown
Contributor Author

@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.

@henrydingliu - done.

@github-actions

github-actions Bot commented Aug 22, 2026

Copy link
Copy Markdown

Pyright Type Completeness

View the full pyright --verifytypes output for this commit

Project (full chainladder package, at this PR's head): 14.9% of exported symbols fully typed (202 / 1356)

Known Ambiguous Unknown Total
Project (head) 202 111 1043 1356

Other symbols referenced but not exported by chainladder: 13

Known Ambiguous Unknown Total
Other (head) 3 1 9 13

Symbols without documentation:

  • Functions without docstring: 324
  • Functions without default param: 0
  • Classes without docstring: 10

Patch (exported symbols added or changed by this PR): no exported symbol type-completeness changes detected.

@codecov

codecov Bot commented Aug 22, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.32%. Comparing base (fee267d) to head (f84014d).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1226   +/-   ##
=======================================
  Coverage   91.32%   91.32%           
=======================================
  Files          92       92           
  Lines        5438     5438           
  Branches      700      700           
=======================================
  Hits         4966     4966           
  Misses        336      336           
  Partials      136      136           
Flag Coverage Δ
unittests 91.32% <ø> (ø)

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

☔ View full report in Codecov by Harness.
📢 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

Copy link
Copy Markdown
Member

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.

@salexanian

Copy link
Copy Markdown
Contributor Author

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 cl.model_diagnostic and provide you with an updated version.

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.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

There are 4 total unresolved issues (including 2 from previous reviews).

Fix All in Cursor

❌ 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 741f4cc. Configure here.

Comment thread docs/friedland/chapter_7_part_2a.ipynb Outdated
@salexanian
salexanian marked this pull request as ready for review August 26, 2026 13:45
@salexanian

Copy link
Copy Markdown
Contributor Author

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.

I have refactored the functions to use model_diagnostic, @henrydingliu .

@henrydingliu

Copy link
Copy Markdown
Member

@salexanian I'm not seeing any usage of model_diagnostics in this latest version

@salexanian

Copy link
Copy Markdown
Contributor Author

@salexanian I'm not seeing any usage of model_diagnostics in this latest version

@henrydingliu it is used in Ex3Sht1(), Ex3Sht10(), Ex4Sht1() and Ex4Sht6() (the other sheets were developed with your dev_exihibit function which already uses cl.model_diagnostic). My use of model_diagnostic is consistent throughout the functions - I generate diag_df (the model_diagnostics dataframe), and then obtain the columns of the tables from that dataframe. In 3 of the functions I stored the scenarios from cl.model_diagnostic separately in a dictionary and obtained the info needed for the columns from there.

@henrydingliu

Copy link
Copy Markdown
Member

this is what I see when i click into the RTD link

image

@salexanian

Copy link
Copy Markdown
Contributor Author

this is what I see when i click into the RTD link

image

I believe that is the function in the original notebook chapter_7_part_2.ipynb, but I put my work into the notebook chapter_7_part_2a.ipynb - I believe you asked me to store my changes in a separate notebook. The amended functions should be in that notebook.

@henrydingliu

Copy link
Copy Markdown
Member

ah, you are right. i completely forgot. my bad.

@henrydingliu

Copy link
Copy Markdown
Member

a few comments:

  • the helper functions format_col etc. are no longer necessary.
  • ibnr is already available in model_diagnostics
  • there are still places where the ultimate is being derived via col_mult, rather than using a chainladder implementation
  • ex3sht1 is handling too many scenarios at the same time. you can massively simplify if you let it just handle a single scenario for each call
  • ex3sht10, etc. should take tri.loc["Steady State"], rather than ("Steady State", tri). the string "Steady State" is also available via tri.loc["Steady State"].index

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants