Repository navigation
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1303 +/- ##
==========================================
- Coverage 94.15% 94.05% -0.10%
==========================================
Files 96 97 +1
Lines 5852 5976 +124
Branches 739 776 +37
==========================================
+ Hits 5510 5621 +111
- Misses 221 227 +6
- Partials 121 128 +7
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:
|
acb0c9f to
bf0e00a
Compare
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): 21.9% fully typed (7 / 32); 3 no longer exported
Patch symbol details
|
acce607 to
cab8a13
Compare
|
Some general thoughts on your approach:
I have an attempt at a general approach in #601, starting with this comment. What I tried to do was examine how a Pandas DataFrame initialized its columns: And see if we can port over the logic. One strategy I would suggest trying is changing |
|
I went through your comments in #601 and the Pandas AxisProperty references. Storing For Triangle.index, what are your thoughts on keeping it stored as a pd.DataFrame vs. migrating to pd.Index/pd.MultiIndex? Currently, Triangle.index returns a DataFrame across the public API (which estimators and TriangleGroupBy depend on for column-based slicing and In the meantime, I will proceed with updating |
35a7872 to
617fc4e
Compare
For 0.11.0, we should retain it as a DataFrame, otherwise it'll lead to a breaking change. I think we should switch to Index/MultiIndex for 1.0.0 - you could make a new issue for that if there isn't one already. I'm not sure if there's a good way to notify the public of this change other than to have it be part of a major version bump. It would be too annoying to warn a user every time they call the index. Anyhow, let me know when you're ready for a review. |
|
I also don't think the |
Makes total sense. Keeping
Agreed. removal is much cleaner since they were never part of the public API, and it saves us having to maintain deprecation shims for internal attributes. I am just clearing up a couple of CI checks on this branch (Read the Docs and coverage) and will ping you here as soon as it is ready for your review. |
|
All checks passed. @genedan this is ready for review. |
| obj = X.copy() | ||
| if X.key_labels == ["Total"]: | ||
| obj.kdims = np.arange(self.n_sims) | ||
| obj._index = pd.DataFrame({"Simulation_#": np.arange(self.n_sims)}) |
There was a problem hiding this comment.
We should avoid accessing _index directly.
| assert tri.index.equals(original_index) | ||
|
|
||
|
|
||
| def test_vdims_deprecation_warning(raa): |
There was a problem hiding this comment.
We can remove the tests for deprecation warnings since we'll be removing the dims without going through the deprecation cycle.
| self._set_slicers() | ||
|
|
||
| @property | ||
| def kdims(self): |
There was a problem hiding this comment.
We can remove the dims properties.
|
|
||
| self.kdims, key_idx = self._set_kdims(data_agg, index) | ||
| self.vdims = np.array(columns) | ||
| kdims_arr, key_idx = self._set_kdims(data_agg, index) |
There was a problem hiding this comment.
looks like the _set_kdims method still exists, we should at least rename it to _set_index (assuming we keep the function).
There was a problem hiding this comment.
Left this for now since we focused on columns first. I'll clean it up when we tackle index next (and avoid stepping on the existing _set_index helper in base.py).
There was a problem hiding this comment.
Let's try not to define the self._index attribute. In a similar way to how we defined the _set_columns helper in axis.py, let's advance to defining _set_index.
Ideally we'll want to have index = TriangleAxis(0, fset=_set_index, doc="Represents the index axis of the triangle.").
Anyhow, this step might be a bit tricky, let me know how it goes.
There was a problem hiding this comment.
Let's rename the following terms to remove the "kdims" terminology:
kdims_arr->index_valueskey_idx->index_idx_set_index->_factorize_index
The first and last of these are consistent with Pandas terminology. The middle one is pretty iffy to me, if you have a strong opinion on it, let me know. Within the existing _set_index function there is a local variable called kdims which I think should be renamed to unique_keys.
|
I've submitted some comments, but consider it a partial review to help guide you where we need to go. This is one of the larger issues to close out, so it will most likely get it take a lot of back and forth between us until we can merge it into main (more commits credited to you!). We'll have to take it step-by-step, but I think we can get it done. Some general goals:
Refactoring this block will be key to closing out the issue: chainladder-python/chainladder/core/triangle.py Lines 561 to 572 in 6b51f64 Ideally, we'd have something like: self.index = TriangleAxis("index", fset=_set_index)
self.columns = TriangleAxis("columns", fset=_set_columns)
self.origin = TriangleAxis("origin", fget=_get_origin, fset=_set_origin)
self.development = TriangleAxis("development", fget=_get_development, fset=_set_development)I have a rough sketch of what class TriangleAxis:
"""Generalized class for representing a Triangle dimension,
analogous to Pandas AxisProperty."""
def __init__(self, key, *, fget=None, fset=None, doc=None):
self.key = key
self.fget = fget # raw -> public; identity if None
self.fset = fset # (obj, public) -> raw; identity if None
self.__doc__ = doc
def __get__(self, obj, objtype=None):
if obj is None:
return self
raw = obj._axes[self.key]
return self.fget(obj, raw) if self.fget else raw
def __set__(self, obj, value):
raw = self.fset(obj, value) if self.fset else value
obj._axes[self.key] = raw
obj._set_slicers()With these helper functions: def _set_index(obj, value):
if not isinstance(value, pd.DataFrame):
raise TypeError("index must be a pandas DataFrame")
obj._len_check(obj.index, value)
return value.copy().reset_index(drop=True)
def _set_columns(obj, value):
if isinstance(value, str):
value = [value]
obj._len_check(obj.columns, value)
return pd.Index(value, name="columns")
def _get_origin(obj, raw):
if obj.is_pattern and len(raw) == 1:
return pd.Series(["(All)"])
freq = {"S": "2Q", "H": "2Q"}.get(obj.origin_grain, obj.origin_grain)
freq = freq if freq == "M" else freq + "-" + obj.origin_close
return pd.DatetimeIndex(raw, name="origin").to_period(freq=freq)
def _set_origin(obj, value):
obj._len_check(obj.origin, value)
freq = {"S": "2Q"}.get(obj.origin_grain, obj.origin_grain)
freq = freq if freq == "M" else freq + "-" + obj.origin_close
return pd.PeriodIndex(list(value), freq=freq).to_timestamp().valuesYou may need to tweak these and this might take trial and error from the both of us. How about we give it a go, create a new |
|
Thanks for the thorough review and guidance,
Completely agree. Removing
Spot on. Reaching into
Nice plan. I really like the Will push an update shortly so we can take a look.. |
|
Whew, looks like the PR is still intact. This was definitely one of the more significant ones that we wanted in. 
|
|
I'm back, really back. Thanos snap |
|
@genedan I pushed an update with the first step of the descriptor refactor. The new Take a look when free. If this shape works I'll follow up with index next. |
|
And, one thing I wanted to add relevant to the last comment is that the |
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 9f18cf4. Configure here.
| self.resampled_triangles_, self.scale_ = self._get_simulation( | ||
| X, exp_incr_triangle | ||
| ) | ||
| # n_obs = xp.nansum(self.w_) |
There was a problem hiding this comment.
Restored the degrees of freedom comment block in bootstrap.py.
25108c4 to
9bf1b65
Compare
…tests and ruff formatting
9bf1b65 to
4df8368
Compare
2 participants
Summary of Changes
Related GitHub Issue(s)
Closes #1010, closes #1011, refs #1053, refs #1216
AI/LLM Usage
Used basic model for initial unit test drafting and lint checks.
Checklist
uv run pytest) and documentation changes (uv run --directory docs jb build . --builder=custom --custom-builder=doctest)Note
High Risk
This is a core Triangle metadata refactor touching indexing, concat, arithmetic, and serialization; regressions could misalign segments or break saved objects despite pickle migration and broad test coverage.
Overview
Replaces Triangle
kdims/vdimswith pandas-styleindex(DataFrame) andcolumns(Index), backed by a newTriangleAxisdescriptor and internal_axesstorage. Construction, slicing, arithmetic alignment, groupby/agg, column assignment, andconcatnow read and write these axes instead of numpy key/value dimension arrays;key_labelsis derived fromindexcolumns.Adds axis helpers (
_get_axis_number/_get_axis_name),Triangle.axes, stricterindex/columnssetters with length checks, boolean Series indexing,sort_index(inplace=True), and scalar broadcast on__setitem__. Pickle/JSON paths gain__setstate__migration from legacykdims/vdims(with warning) and fixes soindexround-trips as a DataFrame. Library estimators (bootstrap, correlation, tails, etc.) and tests are updated to the new API.Reviewed by Cursor Bugbot for commit 4df8368. Bugbot is set up for automated code reviews on this repo. Configure here.