Fixes valuation grain bug - #1194
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): 100.0% fully typed (4 / 4)
Patch symbol details
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1194 +/- ##
=======================================
Coverage 91.14% 91.14%
=======================================
Files 91 91
Lines 5365 5367 +2
Branches 681 682 +1
=======================================
+ Hits 4890 4892 +2
Misses 340 340
Partials 135 135
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}
| # checks if development has any non-yearly values | ||
| dev_has_no_month = not bool( | ||
| development | ||
| and not data[development[0]].astype(str).str.fullmatch(r"\d{4}").all() |
There was a problem hiding this comment.
if pandas parses the development column as floats or with missing values (e.g. 2008.0), .astype(str) yields "2008.0".
that will fail fullmatch(r"\d{4}") and default back to 'M'. stripping trailing .0 or converting to int first would make it a bit safer.
There was a problem hiding this comment.
Sorry, I don't follow you here, you are saying if the data in the column actually says 2008.0? This is not a valid date string format?
There was a problem hiding this comment.
yeah agree it's not valid date formatting!
it's just a pandas edge case where integer year columns with NaNs get cast to float64 (2008.0). so .astype(str) gives "2008.0", failing r"\d{4}" and defaulting grain to 'M'. super minor edge case though!
There was a problem hiding this comment.
Can you give me an actual example? Either with a constructed pandas DF or CSV? I'm having trouble seeing this
There was a problem hiding this comment.
Here's a concrete one. Same annual data as my test, but the dev column comes in as float64 (which is what you often get from a CSV/Excel read, or any time that column lands as float64):
import pandas as pd, chainladder as cl
df = pd.DataFrame({
"origin": [1998, 1999, 2000, 2001, 2002],
"dev": [2008.0, 2008.0, 2008.0, 2008.0, 2008.0], # float64
"expense":[890000, 1170000, 1265000, 1600000, 1200000],
})
tri = cl.Triangle(df, origin="origin", development="dev",
columns="expense", development_format="%Y", cumulative=True)
print(tri.development_grain) # -> 'M', but with int years it's 'Y'
The regex runs on the raw column, and astype(str) turns 2008.0 into "2008.0", which fails \d{4}. It then drops into the else branch, and since %Y parses to Jan 1 (year start), the month-end vs year-end alignment check fails and it picks M. Casting the column first, e.g. data[col].astype("Int64").astype(str), or stripping a trailing .0, fixes it.
There was a problem hiding this comment.
That's a very intentional way wrongly coding dates lol, but easy fix, I'll do that, thank you!
There was a problem hiding this comment.
Eh something broke, let me put this in draft for now. Will fix later
| if len(development_date.unique()) == 1: | ||
| if len(data) == 1 and self.origin_grain.split("-")[0] in ["Y", "A"]: | ||
| # checks if development has any non-yearly values | ||
| dev_has_no_month = not bool( |
There was a problem hiding this comment.
optional nitpick: the double negative not bool(development and not ...) is a bit hard to follow at first glance. might be clearer as an if/else block, but your call!
There was a problem hiding this comment.
Thanks, will rewrite this, good call here. I'll also improve it to loop over it instead of checking only the first index. Though I don't think this is really necessary
There was a problem hiding this comment.
The rewrite reads a lot better, and good call looping over all the development columns. One question: Shall I keep flagging small readability nitpicks, or focus only on functional and correctness issues?
There was a problem hiding this comment.
That's up to you.
For me, I often rely on AI as little as possible, or even use weaker models on purpose to force me to learn (and because I'm cheap/poor lol). I sense that both these comments have some degree of AI involvement there, and I have no problem with that, but I personally wouldn't use AI to bring these up or catch these myself. I guess my point is if you think it's useful to have these discussions (I think yes to some degree) and you are learning, then yes bring them up. I did learn something through these discussions though. Now should we thrive for perfection and resolve all of bots comments 100%? I think that could be silly and pointless. Just my humble opinion :)
If you read the governing doc, I try to put this in there. We don't need to be perfectionists, if someone runs into a bug on production (the other comment) then yes we should def fix it.
There was a problem hiding this comment.
thanks kenneth! i checked out the governing doc and completely agree with prioritizing real production bugs over perfectionism on small details.
i do use AI to cross verify ideas while learning, but your tips are super helpful. i'll definitely take your advice to stick to smaller models so i can learn more on my own!
thanks again for sharing your perspective, i'll stick strictly to functional correctness for future reviews!
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 dd248a1. Configure here.
|
@priyam0k this is ready now, the way that I'm handling it is to cast to int and then string, kinda ugly and not sure if there's a better way. But I added your example as a test too. By the way on the other commend re AI use. Please don't feel like I don't appreciate the feedback, I actually do, and I learned something, but I leave it up to you to decide on how much you use it and what feedback you want to share from your future work. |
nice way when cleaning up float-represented year columns, so nothing ugly. Glad the example was helpful.
Never, I took all your note completely as constructive feedback. Since the experience gap is real, I always value hearing your perspective I'm always looking to try (and even fail) mutiple times, so one day i can finally master it. Here is one meme I often relate, :) on small nitpicks it is so discretionary to pick up relevant ones, I think only experience can reduce it, ... |
|
Sounds good and thanks for the review here! @henrydingliu @genedan can one of you do another secondary review? Will need this merged to unblock #1121, thanks guys! |
3 participants
Summary of Changes
If valuation_date is of length 1 (there's only 1 diagonal), and it contains no month info, it will be defaulted to match origin grain instead
Related GitHub Issue(s)
Fixes #1192
Additional Context for Reviewers
Should be pretty easy to review, added 3 new tests.
Checklist
uv run pytest) and documentation changes (uv run --directory docs jb build . --builder=custom --custom-builder=doctest)Note
Medium Risk
Changes core triangle construction and val/dev conversion logic used across the library; behavior shifts for single-diagonal inputs but is covered by new regression tests.
Overview
Fixes valuation / development grain when every row shares one development date (fixes #1192). Triangles with a single diagonal no longer default to monthly grain when development values are year-only (e.g.
2008or%Y).Triangle.__init__now treats development as year-only when all development column values match\d{4}, or when there is only one data row—thendevelopment_grainmatchesorigin_grain. If month-level information exists, it still compares the parsed date to the origin period end before choosing monthly vs origin grain.val_to_dev/_val_devalways recomputes development-column width from coordinate extents when the array has density, not only when negative development indices need sliding—so single-diagonal triangles keep the correct lag dimension.The
friedland_gl_self_insurersample manifest dropsdevelopment_format: "%Y-12-31so loading uses the same inference rules. New tests cover annual vs monthly single-diagonal cases and the friedland sample’s2008-12-31valuation date.Reviewed by Cursor Bugbot for commit bac78c5. Bugbot is set up for automated code reviews on this repo. Configure here.