Repository navigation
feat(core): deprecate vdims (#1011) and kdims (#1010) - #1303
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.34% 94.34% -0.01%
==========================================
Files 96 97 +1
Lines 5973 6131 +158
Branches 768 798 +30
==========================================
+ Hits 5635 5784 +149
- Misses 219 223 +4
- Partials 119 124 +5
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): 20.0% fully typed (7 / 35); 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. |
4df8368 to
7e5ff84
Compare
|
Last thing would be doing 1 import per line per the above comment, then I'll approve. |
priyam0k
left a comment
priyam0k
left a comment
There was a problem hiding this comment.
Split them into individual import lines.
Grateful for walking through this PR and appreciate your patience, learned a lot here 🙇♂️
|
|
||
| def test_triangle_axis_descriptor(): | ||
| """Verify TriangleAxis descriptor correctly uses integer axis keys for index and columns.""" | ||
| from chainladder.core.axis import TriangleAxis, _set_columns, _set_index |
There was a problem hiding this comment.
Done, split each into its own import line.
|
You can put them inside parentheses, that allows you do avoid typing from chainladder.core.axis import (
_set_columns,
_set_index,
TriangleAxis,
) |
Updated to parenthesized imports |
|
This is probably one of the most complex PRs we've ever done. Good job! |
|
Definitely felt like a puzzle with all the moving parts, glad we made it here |
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 broad axis-metadata refactor touching indexing, arithmetic, concat, and serialization; incorrect alignment could change results or break saved workflows, though extensive tests and pickle migration mitigate breakage.
Overview
Replaces Triangle’s internal
kdims/vdimsstorage with pandas-alignedindex(aDataFrame) andcolumns(Index), backed by a newTriangleAxisdescriptor and_axesdict so axis assignment validates lengths and refreshes slicers.key_labelsis now derived fromindexcolumns (with a setter that renames them), and library code (arithmetic alignment, slicing, groupby/agg,concat, bootstrap simulation indices, JSON I/O) is updated to read/writeindex/columnsinstead of the old arrays. Legacy pickles are migrated in__setstate__with a warning; copy deep-copies axis metadata.Related behavior fixes: boolean
Seriesindexing on the index axis,sort_index(inplace=True), safer column__setitem__(scalars, duplicate labels), and relaxedconcatindex matching (NaNs / differing row labels on axis 1).Reviewed by Cursor Bugbot for commit 814a2cb. Bugbot is set up for automated code reviews on this repo. Configure here.