[ENH] set partial triangle through loc - #1103
Conversation
* enable loc setting on partial triangles * fixing * adding test
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1103 +/- ##
==========================================
- Coverage 91.48% 91.26% -0.22%
==========================================
Files 91 91
Lines 5552 5401 -151
Branches 736 691 -45
==========================================
- Hits 5079 4929 -150
Misses 338 338
+ Partials 135 134 -1
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 / 5)
Patch symbol details
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| 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) |
There was a problem hiding this comment.
Looks like I missed filling out a docstring when I annotated the file. Could you fill it out?
|
How are we doing here with this PR? Can I help? |
|
@kennethshsu that comment I made about the dense version of |
|
@kennethshsu pausing till the sparse test fixture is fixed @genedan i'd like your feedback on
|
|
generalizing across iloc
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 64329cc. Configure here.
| 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) |
There was a problem hiding this comment.
Could you merge in the changes from main? I'd like to see if Ruff catches this line (should be a space before and after the +):
norm_key[1])+(norm_key[2],
There was a problem hiding this comment.
lol, turns out we don't have the technology
There was a problem hiding this comment.
Hmmm looks like it caught other stuff, but not this specific spacing issue I pointed out. I'll need to add the E2 ruleset to the checks.
This Ruff stuff may take us some trial and error to work out. We may want to add/exclude certain rules as we go along. For example E2 is a very broad category of whitespace rules that includes the more granular E225. I think we want most of the things in the E2 category, so that's what I picked - I figure it's easier to go broad and exclude what we don't like than to add very granular rules piecemeal.
|
@kennethshsu can you please take a look at the failing rtd test? there's no config changes in this PR, so I don't know what could have triggered the rtd fail |
seems to be a bug from rtd. everything is working now. please disregard |
I saw something weird that I think you are not supposed to do. In #1201 you opened a PR from |
checks are still failing because of #1201. |
|
Why do you use PR when you need |
cuz i can do this from my phone while i'm eating lunch |
everything was failing for a while. now it's working again 
|
|
I think it's because Don't PR from |
What's the difference? isn't pr just approval for pull? |
|
I think the main difference is the reappointment of "head". If your dev feature has a check failure, that will show up on |
|
And also if you open a PR (from main into dev) all code owners get spammed lol |
|
roger roger |
3 participants
Summary of Changes
adding support to set partial triangles via loc and iloc
adding support to set non-contig index and columns via loc and iloc
fixing a bug where slice(none:none:2) would become slice(none:none:none) during setting.
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
Medium Risk
Changes core Triangle indexing write paths and array mutation semantics; scope is bounded to numpy backend with new tests, but incorrect
np.ix_alignment could corrupt triangle data.Overview
Extends numpy-backend
loc/ilocassignment so you can write partial triangles (origin/development ranges and labels) and copy non-contiguous index/column selections—behavior that previously worked on read but not on write.The setter now runs keys through
_contig_slice(same as the getter), usesnp.ix_when both index and column axes are fancy arrays (matching the getter’s Cartesian product), and rejects fancy indexing on origin/development with a clear error._normalize_indexno longer drops explicit slice steps (e.g.::2), which had broken strided assignments.Minor cleanups:
is notfor type checks,np.where(check)without== True, and docstrings. Tests cover partial triangle sets, non-contiguousloc/ilocsets, and set-side fancy origin/development errors (numpy backend only; sparse still requires.at/.iat).Reviewed by Cursor Bugbot for commit 38765f1. Bugbot is set up for automated code reviews on this repo. Configure here.