Skip to content

#928 quarterly warning#929

Closed
genedan wants to merge 6 commits into
mainfrom
#928-quarterly_warning
Closed

#928 quarterly warning#929
genedan wants to merge 6 commits into
mainfrom
#928-quarterly_warning

Conversation

@genedan
Copy link
Copy Markdown
Collaborator

@genedan genedan commented Jun 5, 2026

Summary of Changes

Handles quarterly edge case when conducting datetime inference. Python doesn't have a native datetime format string for quarterly dates, so I decided to intercept the UserWarning and handle the quarterly case separately.

Reduces the number of warnings from 1112 to 1080.

Related GitHub Issue(s)

#840
#928

Additional Context for Reviewers

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

Note

Medium Risk
Changes core Triangle date parsing used at construction; quarterly fallback could affect edge-case origin/development labels, though behavior for known formats should stay the same.

Overview
TriangleBase._to_datetime now treats UserWarning from pd.to_datetime as a failed inference attempt (via warnings.filterwarnings('error')) instead of letting quarterly strings warn and still partially parse. When the usual format list fails, it falls back to pd.PeriodIndex(..., freq='Q').to_timestamp, honoring period_end through a shared start_end flag for period timestamps.

A regression test asserts Triangle still raises ValueError with "Unable to infer datetime" when origin/development strings use incompatible formats (e.g. 1995/Q1 vs 1995Q1). Triangle docs only clarify the data type as DataFrame | DataFrameXchg | dict.

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

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 5, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.32%. Comparing base (31b3432) to head (99682fe).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #929      +/-   ##
==========================================
+ Coverage   88.28%   88.32%   +0.03%     
==========================================
  Files          88       88              
  Lines        5029     5037       +8     
  Branches      642      642              
==========================================
+ Hits         4440     4449       +9     
+ Misses        444      443       -1     
  Partials      145      145              
Flag Coverage Δ
unittests 88.32% <100.00%> (+0.03%) ⬆️

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.

Copy link
Copy Markdown

@cursor cursor Bot left a comment

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 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 402a080. Configure here.

Comment thread chainladder/core/base.py Outdated
@henrydingliu
Copy link
Copy Markdown
Collaborator

I had the entirely wrong idea of what this PR would look like. I was honestly thinking you would just change the underlying data.

  • switching from a warning to an error should probably take a deprecation cycle?
  • i'm not too hot about adding these edge cases. sure, it's one of the sample datasets in the package. but elsewhere in the repo, we have non-annual grains dataset working perfectly fine. What if someone now wants to open up an issue to add an edge case for H, or another date format? I think my response to any of those issues would be, "wrangle in pandas", which I believe is also sklearn's position.

@genedan
Copy link
Copy Markdown
Collaborator Author

genedan commented Jun 5, 2026

I can try that. It would be something like replacing data like 1995-Q2 with 1995-04 to fix? I'll open up another PR if I can get it to work.

Is there anywhere in the docs that has best practice for the date formats? The docs are quite hard to search. If absent, we could add that the best way to format a quarterly triangle would be to have the month beginning of the quarter rather than using a Q, which could be inferred but will cause performance issues.

@genedan
Copy link
Copy Markdown
Collaborator Author

genedan commented Jun 5, 2026

I think the term I was looking for was ISO 8601, we could update the docs to say the best practice would be to encode the dates as ISO 8601 compatible.

@henrydingliu
Copy link
Copy Markdown
Collaborator

Is there anywhere in the docs that has best practice for the date formats? The docs are quite hard to search. If absent, we could add that the best way to format a quarterly triangle would be to have the month beginning of the quarter rather than using a Q, which could be inferred but will cause performance issues.

There is this. We can definitely beef it up.

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