Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1303 +/- ##
==========================================
- Coverage 92.30% 92.25% -0.05%
==========================================
Files 100 101 +1
Lines 5793 6029 +236
Branches 713 803 +90
==========================================
+ Hits 5347 5562 +215
- Misses 325 339 +14
- 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): 26.3% fully typed (5 / 19); 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 |
…tests and ruff formatting
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).
|
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 |
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.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f711b88. Configure here.
|
@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. |
genedan
left a comment
There was a problem hiding this comment.
This is starting to look way cleaner, good work. I left some comments requesting some changes, and I asked a few questions too.
This is going to be an awesome PR!
| db = None | ||
|
|
||
| from typing import cast, Optional, TYPE_CHECKING | ||
| from typing import Any, cast, Optional, TYPE_CHECKING |
There was a problem hiding this comment.
Let's put these on multiple lines.
|
|
||
|
|
||
| class TriangleAxis: | ||
| """ |
There was a problem hiding this comment.
Add docstrings for:
- The class (add parameters)
- Methods
| *, | ||
| fget: Callable[[Triangle, Any], Any] | None = None, | ||
| fset: Callable[[Triangle, Any], Any] | None = None, | ||
| doc: str | None = None, |
There was a problem hiding this comment.
I think this doc parameter might be extraneous. Compare these two outputs:
import chainladder as cl
import pandas as pd
pd.DataFrame().columns.__doc__
raa = cl.load_sample('raa')
raa.columns.__doc__The both resolve to the doc for an Index object, which is right. The raa.columns.__doc__ mentions Pandas though, I wonder if there's an easy way to replace the word "pandas" with "chainladder" without too much engineering?
Seeing "pandas" in the doc probably shouldn't be a big deal for now though, let's put it low on the priority list of changes but nice to have if you can get around to it.
| *args, | ||
| **kwargs, | ||
| ): | ||
| self._axes: dict[str, Any] = {} |
There was a problem hiding this comment.
We should turn this into a public property that returns a list of the access. Doing so would match the Pandas signature:
@property
def axes(self) -> list[Index]:
"""
Return a list representing the axes of the DataFrame.| X._index = self._index.copy() | ||
| return X | ||
|
|
||
| def __setstate__(self, state: dict) -> None: |
There was a problem hiding this comment.
Will this block still be necessary once you're done with the PR? A passing test at the end of step 2 of this PR is enough to convince me that we haven't broken anything. We can remove this block and the associated test in a new PR step 3.
| if isinstance(obj, TriangleGroupBy): | ||
|
|
||
| def f(k, self, obj, other): | ||
| # fmt: off |
| if isinstance(obj, TriangleGroupBy): | ||
|
|
||
| def f(k, self, obj, other): | ||
| # fmt: off |
| rows = X.index.set_index(X.key_labels).index | ||
| self.omega_ = pd.DataFrame(params[..., 0, 0], index=rows, columns=X.vdims) | ||
| self.theta_ = pd.DataFrame(params[..., 0, 1], index=rows, columns=X.vdims) | ||
| self.omega_ = pd.DataFrame( |
There was a problem hiding this comment.
Do we need to use columns_label here, or can we extract it from the new columns index?
| fitted = xp.repeat(fitted, self.ldf_.shape[2], axis=2) | ||
| rows = X.index.set_index(X.key_labels).index | ||
| self.b_ = pd.DataFrame(self.b_[..., 0, 0], index=rows, columns=X.vdims) | ||
| self.b_ = pd.DataFrame(self.b_[..., 0, 0], index=rows, columns=X.columns_label) |
There was a problem hiding this comment.
Same as the other comment, can we extract from columns index?
| rows = self.ldf_.index.set_index(self.ldf_.key_labels).index | ||
| return pd.DataFrame( | ||
| self._slope_[..., 0, 0], index=rows, columns=self.ldf_.vdims | ||
| self._slope_[..., 0, 0], index=rows, columns=self.ldf_.columns_label |
Summary of Changes
Related GitHub Issue(s)
Closes #1010, closes #1011, refs #601, refs #1216
Additional Context for Reviewers
Existing code reading or setting vdims and kdims continues to work as expected for backward compatibility. Date dimensions (odims and ddims) will follow in a separate PR.
Checklist
uv run pytest) and documentation changes (uv run --directory docs jb build . --builder=custom --custom-builder=doctest)Note
High Risk
This is a cross-cutting change to Triangle’s dimension model (indexing, I/O, arithmetic, and serialization); regressions could affect most user workflows even with pickle migration and broad test updates.
Overview
Migrates Triangle row/column metadata off
kdims/vdimsonto pandas-styleindex(_indexDataFrame) andcolumns, with a newTriangleAxisdescriptor backing the value dimension.Triangle construction, slicing, arithmetic alignment, groupby/agg,
concat, JSON I/O, and estimator outputs now read and writeindex/columns(andkey_labelsvia_indexcolumns) instead of numpykdims/vdims. Pickle__setstate__migrates legacy serialized triangles;copy()isolates_index; index/column setters validate length and refresh slicers. Bootstrap resampling builds simulation keys by extendingTriangle.indexwith aSimulation_#column.sort_index(inplace=True)mutates values and index together; booleanSeriesindexing is supported.Tests cover columns/
key_labelssetters, legacy pickle shapes, JSON roundtrip of_index, andconcat(..., ignore_index=True)on axes 1–3.Reviewed by Cursor Bugbot for commit 7f4e4d1. Bugbot is set up for automated code reviews on this repo. Configure here.