Skip to content

[FEAT] Centralize axis descriptors - #1454

Open
priyam0k wants to merge 3 commits into
feature/deprecate-date-dimsfrom
feature/centralized-axis-mapping
Open

priyam0k wants to merge 3 commits into
feature/deprecate-date-dimsfrom
feature/centralized-axis-mapping

Conversation

@priyam0k

@priyam0k priyam0k commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Summary of Changes

  • Centralizes axis order tuples and lookup mappings in utility_functions and re-exports in core.typing.
  • Migrates origin and development to TriangleAxis descriptors, retiring separate internal dimension storage into _axes.
  • Implements Triangle._set_axis to generalize dimension validation and assignment.
  • Adds TrianglePandas._get_axis_number and routes _get_axis to eliminate redundant local lookup dicts.
  • Adds comprehensive unit tests for _set_axis and legacy pickle unpickling.

Related GitHub Issue(s)

Closes #1053

AI/LLM Usage

Used basic model for initial unit test drafting and lint checks.

Additional Context for Reviewers

Follow-up to Issue #1053, stacked directly on top of #1304 (Stack #1437). Can be reviewed here and retargeted to main once Stack #1437 finishes merging.

Submitter's Checklist

  • I have reviewed and am adhering to the standards outlined in the project Governing Doc.
  • The PR subject title summarizes the changes, with one proper prefix ([FIX], [FEAT], [DOCS], [TST], [CHORE], or [BRK]).
  • I am a human (not a bot), and this PR form is written by a human.

Reviewer's Checklist

  • The implementation addresses the associated issue(s).
  • The implementation is appropriate, maintainable, and follows ARCHITECTURE.md.
  • PR subject title has the proper prefix and the subject is appropriate.
  • Relevant issue(s) are linked.
  • AI/LLM usage is disclosed and appropriate.
  • Documentation and tests are appropriate.
  • CI tests passed, or any failures are acceptable.
  • Leave a comment with the final recommendation (e.g. approve as is, request a secondary review, or flag an area for more review).

Note

Medium Risk
Touches core Triangle axis storage, assignment, and unpickling paths; behavior is intended to be preserved but regressions could affect slicing, renaming, and serialized workflows.

Overview
Centralizes Triangle axis handling by moving shared name/number resolution into utility_functions (re-exported from core.typing) and routing TrianglePandas._get_axis_number / TriangleBase._get_axis_value through those helpers instead of local dicts.

Migrates origin and development to TriangleAxis descriptors (like index/columns): getter/setter logic lives in chainladder/core/axis.py, raw timestamps/lags are stored in _axes[2]/_axes[3] via _odims/_ddims setters, and Triangle._set_axis assigns any axis by name or index.

Pickle __setstate__ now normalizes legacy odims/ddims/origin/development keys into _axes and drops redundant state. Tests cover _set_axis, descriptor edge cases (patterns, valuation triangles), and expanded legacy unpickling.

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

@priyam0k
priyam0k added this pull request to stack #1437 October 2, 2026 13:17
@github-actions

github-actions Bot commented Oct 2, 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): 15.6% of exported symbols fully typed (232 / 1488)

Known Ambiguous Unknown Total
Project (head) 232 108 1148 1488

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: 327
  • Functions without default param: 0
  • Classes without docstring: 8

Patch (exported symbols added or changed by this PR): 50.0% fully typed (2 / 4)

Known Ambiguous Unknown Total
Patch 2 0 2 4
Patch symbol details
Symbol Status Change
chainladder.core.tests.test_triangle.test_centralized_axis_edge_cases ❌ unknown new
chainladder.core.tests.test_triangle.test_set_axis ❌ unknown new
chainladder.core.triangle.Triangle.development ✅ known changed (was ❌ unknown)
chainladder.core.triangle.Triangle.origin ✅ known changed (was ❌ unknown)

@codecov

codecov Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.80220% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.12%. Comparing base (5668b6d) to head (deecc2a).

Files with missing lines Patch % Lines
chainladder/core/axis.py 94.73% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@                       Coverage Diff                       @@
##           feature/deprecate-date-dims    #1454      +/-   ##
===============================================================
+ Coverage                        94.07%   94.12%   +0.04%     
===============================================================
  Files                               97       97              
  Lines                             5996     6044      +48     
  Branches                           778      790      +12     
===============================================================
+ Hits                              5641     5689      +48     
+ Misses                             227      226       -1     
- Partials                           128      129       +1     
Flag Coverage Δ
unittests 94.12% <97.80%> (+0.04%) ⬆️

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.

@priyam0k
priyam0k marked this pull request as ready for review October 2, 2026 14:31
@priyam0k
priyam0k force-pushed the feature/centralized-axis-mapping branch from dec0dbe to 2e30a85 Compare October 2, 2026 23:08
@priyam0k
priyam0k force-pushed the feature/centralized-axis-mapping branch from 2e30a85 to 656767a Compare October 3, 2026 06:47
@priyam0k
priyam0k force-pushed the feature/centralized-axis-mapping branch 2 times, most recently from c1996e6 to de7b967 Compare October 3, 2026 08:28
@priyam0k priyam0k mentioned this pull request Oct 3, 2026
3 of 11 tasks
@priyam0k
priyam0k force-pushed the feature/centralized-axis-mapping branch from de7b967 to 1c2d341 Compare October 3, 2026 15:57
@priyam0k
priyam0k removed this pull request from stack #1437 October 3, 2026 16:54
@priyam0k
priyam0k added this pull request to stack #1459 October 3, 2026 16:55
@priyam0k
priyam0k force-pushed the feature/centralized-axis-mapping branch from 1c2d341 to e0659b6 Compare October 7, 2026 03:33
@priyam0k
priyam0k force-pushed the feature/centralized-axis-mapping branch 2 times, most recently from eb6567d to 5d1a3c6 Compare October 9, 2026 14:38
@priyam0k
priyam0k force-pushed the feature/centralized-axis-mapping branch from 5d1a3c6 to deecc2a Compare October 11, 2026 17:16
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.

[DISCUSSION] Create centralized axis mapping

1 participant