[Points] Fix events.data_indices for ActionType.ADDED event when adding sing 8000 le point #7983
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7983 +/- ##
==========================================
+ Coverage 92.89% 92.94% +0.05%
==========================================
Files 643 647 +4
Lines 60688 60838 +150
==========================================
+ Hits 56375 56545 +170
+ Misses 4313 4293 -20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Nice catch @Llewi ! And thanks for opening this PR! For anyone wanting a bit more details, for a single point, I looked at our tests and lo and behold, we only test the multiple point case, which is why this bug sailed through tests: So I do think ensuring we have a test to cover both cases would be worth it. My thought would be to parameterize the existing test and I'm happy to do that @Llewi unless you want to give it a go. |
|
Hey @psobolewskiPhD , |
|
Sounds good @Llewi |
|
Docs build fail is with fetching a data file: I re-ran the build. |
|
@Llewi, thank you for this bugfix! I appreciate that you took the time to figure this out, make a PR, and be quick to reply! In the future, we can also work with you more closely (via zoom, zulip chat, etc) if you'd like to learn more about tests in napari and make the changes yourself. Either way, grateful for your contribution! |
|
Agreed, awesome comments and review @psobolewskiPhD. I've copied them over to the description so they end up in the commit message. 😊 |
|
Thanks @Llewi! |
Description
(Bugfix) Events for adding/removing points have a data_indices property. This works as expected for ActionType.REMOVING, .REMOVED, and .ADDING.
For ActionType.ADDED, however, a bug occured when only a single point is added (which is the standard case in the GUI). The intention in the original code was to query the number of points via len(coords), but this does not produce the desired result when the coords variable is just a one-dimensional point.
The same np.atleast_2d function that I utilised for the fix was already used in _set_data a few lines above; I hope this fix is trivial enough to pass without test case.
Edit: additional info
Copied by @jni from @psobolewskiPhD's comments (1, 2) below.
For anyone wanting a bit more details, for a single point,
coordsis e.g.[18, 18]but for multiple, it's e.g.[[18, 18], [18, 18]]. So for the single point case, thelenreturns the number of axes, rather than the number of points, so thedata_indexesends up being a e.g. (-2, -1), rather than (-1,). For multiple points it works fine of course. So the fix here is absolutely correct.I looked at our tests and lo and behold, we only test the multiple point case, which is why this bug sailed through tests:
napari/src/napari/layers/points/_tests/test_points.py
Line 1189 in 18b263b
[…]
I parameterized test_add_points_direct to also check adding a single point.
This fails on main but is fixed by this PR.