Skip to content

[ENH] set partial triangle through loc - #1103

Draft
henrydingliu wants to merge 9 commits into
mainfrom
loc_setter
Draft

[ENH] set partial triangle through loc#1103
henrydingliu wants to merge 9 commits into
mainfrom
loc_setter

Conversation

@henrydingliu

@henrydingliu henrydingliu commented Jul 10, 2026

Copy link
Copy Markdown
Member

Summary of Changes

adding support to set triangle values a few origins or devs at a time

Related GitHub Issue(s)

closes #1069

Additional Context for Reviewers

the heavy lifting is done by the existing _contig_slice function, which is already used in the getter. not sure why it was never applied in the setter

  • I passed tests locally for both code (uv run pytest) and documentation changes (uv run jb build docs --builder=custom --custom-builder=doctest)

Note

Medium Risk
Touches core 4-D indexing assignment in slice.py, which can silently mis-assign data if np.ix_ or contiguous-key logic is wrong; scope is numpy-only with explicit guards and new tests.

Overview
Extends Triangle.loc and Triangle.iloc assignment on the numpy backend so you can write partial slices of origin/development periods and non-contiguous index/column selections, matching behavior that getters already supported.

The setter now runs keys through _contig_slice (same as get_idx) before writing values. For fancy index lists on the index/column axes, writes use np.ix_ so assignments align with the getter’s Cartesian-product selection. Fancy indexing on origin or development when setting still raises ValueError; sparse backends remain limited to .at / .iat.

New tests cover partial origin/dev updates, non-contiguous loc/iloc writes, and the new error paths.

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

henrydingliu and others added 4 commits July 2, 2026 07:10
* enable loc setting on partial triangles

* fixing

* adding test
@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.79%. Comparing base (6c9ce99) to head (64329cc).
⚠️ Report is 81 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1103      +/-   ##
==========================================
+ Coverage   90.48%   91.79%   +1.30%     
==========================================
  Files          91       91              
  Lines        5289     6252     +963     
  Branches      671      889     +218     
==========================================
+ Hits         4786     5739     +953     
- Misses        359      363       +4     
- Partials      144      150       +6     
Flag Coverage Δ
unittests 91.77% <100.00%> (+1.28%) ⬆️

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.

@github-actions

github-actions Bot commented Jul 10, 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): 14.5% of exported symbols fully typed (185 / 1274)

Known Ambiguous Unknown Total
Project (head) 185 106 983 1274

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

Patch (exported symbols added or changed by this PR): 0.0% fully typed (0 / 5)

Known Ambiguous Unknown Total
Patch 0 0 5 5
Patch symbol details
Symbol Status Change
chainladder.core.tests.test_slicing.test_iloc_setting_non_contiguous_index_and_columns ❌ unknown new
chainladder.core.tests.test_slicing.test_loc_setitem_partial_triangles ❌ unknown new
chainladder.core.tests.test_slicing.test_loc_setting_non_contiguous_index_and_columns ❌ unknown new
chainladder.core.tests.test_slicing.test_set_fancy_development_raises ❌ unknown new
chainladder.core.tests.test_slicing.test_set_fancy_origin_raises ❌ unknown new

Comment thread chainladder/core/tests/test_slicing.py Outdated
Comment thread chainladder/core/slice.py
return out

def __setitem__(self, key: _LabelKey, values: int | float | TriangleSlicer) -> None:
super().__setitem__(cast(tuple[_AxisKey, _AxisKey, _AxisKey, _AxisKey], self.key_to_slice(key)), values)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like I missed filling out a docstring when I annotated the file. Could you fill it out?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done

Comment thread chainladder/core/slice.py Outdated
Comment thread chainladder/core/tests/test_slicing.py Outdated
@kennethshsu

Copy link
Copy Markdown
Member

How are we doing here with this PR? Can I help?

@genedan

genedan commented Jul 24, 2026

Copy link
Copy Markdown
Member

@kennethshsu that comment I made about the dense version of raa (and other fixtures, possibly) being run twice instead of alternating between dense/sparse as intended could definitely use assistance. It'll need it's own issue created and PR to remedy. I think it might reveal a lot more test failures too once we fix it.

@henrydingliu

Copy link
Copy Markdown
Member Author

@kennethshsu pausing till the sparse test fixture is fixed

@genedan i'd like your feedback on

  1. i'd like to continue to observe John's intent around no loc/iloc setting for sparse.
  2. I'd like to enable non-contig setting if the non-contig dims are index and column.

@henrydingliu
henrydingliu marked this pull request as draft July 29, 2026 22:58
@genedan

genedan commented Jul 30, 2026

Copy link
Copy Markdown
Member
  1. Agreed, while theoretically we could enable the feature there are other things more worth our time and this can remain as-is unless a bunch of users ask for it.
  2. Agreed.

Comment thread chainladder/core/slice.py

@cursor cursor Bot left a comment

Copy link
Copy Markdown

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 64329cc. Configure here.

Comment thread chainladder/core/slice.py
cast(np.ndarray, cast(object, self.obj.values)).__setitem__(norm_key, values)
else:
#the getter uses arr[idx,:][:,idx] to get the Cartesian product, using np.ix_ on the setter to match
cast(np.ndarray, cast(object, self.obj.values)).__setitem__(np.ix_(norm_key[0], norm_key[1])+(norm_key[2], norm_key[3]), values)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stepped slice step dropped

Medium Severity

The new setter runs _contig_slice and then _normalize_index, unlike get_idx, which uses the contig result directly. When an arithmetic index sequence spans a full axis, _contig_slice builds a stepped slice, and _normalize_index then clears both bounds and drops the step, so assignment can write the entire axis instead of the selected positions.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 64329cc. Configure here.

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.

[BUG] Location setter erroring unexpectedly

3 participants