[ENH] set partial triangle through loc#1103
Conversation
* enable loc setting on partial triangles * fixing * adding test
henrydingliu
requested review from
genedan,
jbogaardt and
kennethshsu
as code owners
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1103 +/- ##
=======================================
Coverage 90.48% 90.49%
=======================================
Files 91 91
Lines 5289 5291 +2
Branches 671 671
=======================================
+ Hits 4786 4788 +2
Misses 359 359
Partials 144 144
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:
|
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): 0.0% fully typed (0 / 1)
Patch symbol details
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 de661e2. Configure here.
| raa_new = raa.copy() | ||
| raa_new.loc[:,:,:'1984',:] = raa2.loc[:,:,:'1984',:] | ||
| raa_new.loc[:,:,'1985':,:] = raa2.loc[:,:,'1985':,:] | ||
| assert raa_new == raa2 |
There was a problem hiding this comment.
Sparse parametrization breaks new test
Medium Severity
test_loc_setitem_partial_triangles assigns through Triangle.loc without checking array_backend. The raa fixture is parametrized with sparse_only_run, where _LocBase.__setitem__ always raises when setting via .loc. The sibling test_loc_setitem_triangle_value guards sparse; this test does not, so half the parametrized runs fail.
Reviewed by Cursor Bugbot for commit de661e2. Configure here.
There was a problem hiding this comment.
@genedan cursor's explanation makes sense. but why did all tests pass?
1 participant
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
uv run pytest) and documentation changes (uv run jb build docs --builder=custom --custom-builder=doctest)Note
Low Risk
Small, localized change to label-based assignment on the numpy backend, with tests; no API or sparse-path changes beyond existing restrictions.
Overview
Enables partial writes on origin and development via
Triangle.loc[...] = ...when assigning aTriangleslice, by running label-derived integer keys through_LocBase._contig_slicebefore assignment—the same normalization already used on get.Previously, setter paths could pass non-contiguous index arrays that NumPy assignment could not handle the same way as reads; contiguous slices (e.g. development
...,:60and72:, or origins through'1984'and from'1985') now assign correctly on the numpy backend.Adds
test_loc_setitem_partial_trianglesto assert two-step dev and origin updates reproduce a fully scaled triangle. Sparse backend behavior is unchanged (still requires.at/.iatfor assignment).Reviewed by Cursor Bugbot for commit de661e2. Bugbot is set up for automated code reviews on this repo. Configure here.