Skip to content

nan_triangle 1D logic overhaul#702

Merged
genedan merged 4 commits into
casact:mainfrom
danielfong-act:master
Apr 22, 2026
Merged

nan_triangle 1D logic overhaul#702
genedan merged 4 commits into
casact:mainfrom
danielfong-act:master

Conversation

@danielfong-act
Copy link
Copy Markdown
Contributor

@danielfong-act danielfong-act commented Apr 20, 2026

Fixes #692

  1. Reimplemented is_pattern as a property in Triangle. Adds _pattern as an internal attribute.
  2. Added the abstract properties is_pattern and is_ultimate to TriangleBase. TriangleBase is now an abstract class.
  3. Changed nan_triangle to check explicitly for pattern/ultimate vectors, instead of checking for any 1D Triangles.

see this post. cum_to_incr() mishandles 2x2 triangles because it passes nan_triangle a one dimensional triangle (corresponding to the latter development period), and receives back a full 1D triangle of ones. This causes cum_to_incr() to ignore valuation information and populate the lower right corner of the triangle with a negative incremental value.

In addition to the test suite, I tested this PR with this snippet of code pulled from the issue discussion:

import chainladder as cl
import pandas as pd

df = pd.DataFrame(data={
    'origin': [2022, 2022, 2023],
    'development': [2022, 2023, 2023],
    'reported': [78000, 222000, 78000]}
)

tri_from_df = cl.Triangle(
    data=df,
    origin='origin',
    development='development',
    columns=['reported'],
    cumulative=True
)

tri_from_df

tri_from_df.cum_to_incr()

I also ensured that development estimators that set is_pattern (e.g. ClarkLDF) continue to function properly.

This PR will restrict nan_triangle to only return 1D triangles of ones for pattern and ultimate triangles. If I'm missing a case, please let me know.


Note

Medium Risk
Touches core triangle masking used across arithmetic and conversions, so it may subtly change results for pattern/ultimate workflows even though coverage includes a targeted regression test.

Overview
Fixes a cum_to_incr() regression on 2x2 triangles by changing nan_triangle to return an all-ones mask only for pattern vectors and ultimate vectors, rather than for any 1D triangle.

Promotes TriangleBase to an abstract base class by requiring is_pattern/is_ultimate properties, and reworks Triangle.is_pattern to use an internal _pattern attribute with a property/setter. Adds a regression test covering the 2x2 conversion case.

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

danielfong-act added 2 commits April 20, 2026 14:16
…y with setter. Added is_pattern and is_ultimate abstract properties to TriangleBase
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 20, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.12%. Comparing base (08b9987) to head (a25ea4c).
⚠️ Report is 12 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #702      +/-   ##
==========================================
+ Coverage   85.08%   85.12%   +0.03%     
==========================================
  Files          85       85              
  Lines        4896     4909      +13     
  Branches      629      629              
==========================================
+ Hits         4166     4179      +13     
  Misses        521      521              
  Partials      209      209              
Flag Coverage Δ
unittests 85.12% <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 Sentry.
📢 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.

@genedan genedan requested review from genedan April 21, 2026 22:24
@genedan
Copy link
Copy Markdown
Collaborator

genedan commented Apr 22, 2026

@danielfong-act, thanks for going the extra mile and figuring out the abstract class features! I just have a few minor requests:

  1. Add type hints to the getter and setter (-> bool)
  2. Add a space after self, in line 492

@danielfong-act
Copy link
Copy Markdown
Contributor Author

sure, cleaner code is always better

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 2efef59. Configure here.


@property
def is_ultimate(self):
def is_ultimate(self) -> np.bool:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewer requested -> bool but got -> np.bool

Low Severity

The is_ultimate property uses -> np.bool as the return type annotation. The PR reviewer explicitly requested -> bool type hints for the getter and setter. Additionally, np.bool was removed in numpy 1.24 and only restored in numpy 2.0, making it an unreliable alias. The function actually returns a Python bool (from int > 0), so the annotation is also technically inaccurate. This violates the reviewer's explicit request: "Add type hints to the getter and setter (-> bool)".

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 2efef59. Configure here.

@genedan
Copy link
Copy Markdown
Collaborator

genedan commented Apr 22, 2026

Could you put in three more bool return type hints on these lines?

318
323
493

Also, please add a unit test for a 2x2 triangle. I think the one in #692 would be a good one to use.

@danielfong-act
Copy link
Copy Markdown
Contributor Author

Sure, but does using numpy.bool_ (where appropriate) bother you? e.g. is_ultimate returns it since it uses numpy arrays.

@genedan
Copy link
Copy Markdown
Collaborator

genedan commented Apr 22, 2026

numpy.bool_ works for me.

@danielfong-act
Copy link
Copy Markdown
Contributor Author

danielfong-act commented Apr 22, 2026

I left the setter alone since python really seems to discourage having setters returning anything. I also don't think people expect a return in the context of a property assignment.

@genedan
Copy link
Copy Markdown
Collaborator

genedan commented Apr 22, 2026

Closes #692. Good job!

@genedan genedan merged commit 6cb4aab into casact:main Apr 22, 2026
8 checks passed
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.

cum_to_incr() produces spurious values on triangles produced by .grain().

2 participants