Add `remove()` in Shapes and Points by rahul713rk · Pull Request #8031 · napari/napari

rahul713rk · 2025-06-16T10:43:52Z

References and relevant issues

Description

This PR enhances shape and point layer functionality by introducing new methods and improving existing ones:

➕ Added remove() method to both Shapes and Points layers for explicit item removal
🔧 Modified remove_selected() to use the new remove() internally, improving code clarity and modularity

EDIT : Added tests for remove for both Points and Shapes (test_removing_points , test_removing_shapes )
and Remove pop method from this PR for follow-up issue #8070

…hapes.py`

psobolewskiPhD · 2025-06-20T02:06:59Z

The test fails appear legit, because the event is moved from remove_selected to remove.
I think we want to keep the events the same for the time being.
the question is whether there should also be an event for remove -- but this would risk double events.

…nges

rahul713rk · 2025-06-20T08:23:22Z

@TimMonko @psobolewskiPhD — thank you for pointing out the mistake .
Initially, I thought it was code duplication, which is why I removed it. After revisiting the module, I realized its significance and have now rectified the mistake.
Please let me know if there are any further changes or improvements I should make.

psobolewskiPhD · 2025-06-26T21:24:25Z

@rahul713rk There's some minor merge conflicts, do you think you can resolve them or would you rather one of the maintainers do so?

Edit: also we will need some tests for the new methods. I think you can just copy and modify the existing test, e.g. for points:

napari/src/napari/layers/points/_tests/test_points.py

Line 419 in 1b608e3

def test_removing_selected_points():

psobolewskiPhD · 2025-06-26T21:25:14Z

@jni you had thoughts on this previously, could you take a look at the implementation and see what you think?

TimMonko · 2025-06-29T03:30:46Z

Moving to 0.6.3 so we have time to look at this, which unfortunately I don't have right now! Thanks @rahul713rk for the work you've done so far!

rahul713rk · 2025-07-01T17:41:14Z

Thanks @psobolewskiPhD , @TimMonko for your support and guidance , i have resolved the conflict and add test cases with name test_removing_points , test_pop_points , test_removing_shapes , test_pop_shapes

stale

codecov · 2025-07-01T18:30:07Z

Codecov Report

❌ Patch coverage is 98.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 93.06%. Comparing base (fd7a743) to head (7215479).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
src/napari/layers/shapes/shapes.py	94.44%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8031      +/-   ##
==========================================
+ Coverage   93.00%   93.06%   +0.06%     
==========================================
  Files         702      702              
  Lines       63269    63321      +52     
==========================================
+ Hits        58841    58930      +89     
+ Misses       4428     4391      -37

☔ View full report in Codecov by Sentry.
📢 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.

psobolewskiPhD · 2025-07-03T12:54:50Z

+    def pop(self) -> None:
+        """Remove the last point from the layer."""
+        visible_indices = self._indices_view.tolist()
+        if not visible_indices:
+            # If no points are visible, do nothing
+            show_warning('No points to remove.')
+            return
+        self.remove([visible_indices[-1]])


Do we want pop to work on visible only? I think i lean towards just removing the last item regardless?
On the other hand, there may be some utility in removing from visible items only 🤔
Also is it worth making pop more like the Python List method, where it returns the item? that method also accepts an index for what it's worth...

psobolewskiPhD · 2025-07-03T12:57:20Z

Thanks for resolving the conflicts and your patience @rahul713rk
For me the remove/remove_selected are nice, but I still am uncertain about the pop functionality:

should it be limited to visible, as you have it, or all points
should it also return the point, like Python list pop
should it accept an index?
Please bear with us as we try to get more feedback on that!

brisvag · 2025-07-03T15:21:25Z

Yes, IMO we should aim to stick as close as possible to what pop means in other python contexts (like list). So the signature I would expect is:

	def pop(self, index=-1) -> npt.ArrayLike: # of shape (D,)

And it shouldn't use shown.

psobolewskiPhD · 2025-07-04T00:41:45Z

Another option is to split off the pop method into a new PR and then we can just get remove merged, which would be a nice benefit.

rahul713rk · 2025-07-04T08:30:25Z

Yes, IMO we should aim to stick as close as possible to what pop means in other python contexts (like list). So the signature I would expect is:
	def pop(self, index=-1) -> npt.ArrayLike: # of shape (D,)
And it shouldn't use shown.

shouldn't we return dictionary (for poped item only) containing

 output =  {data = data,
            shape_type=shape_type,
            edge_width=edge_width,
            edge_color=edge_color,
            edge_color_cycle=edge_color_cycle,
            edge_colormap=edge_colormap,
            edge_contrast_limits=edge_contrast_limits,
            face_color=face_color,
            face_color_cycle=face_color_cycle,
            face_colormap=face_colormap,
            face_contrast_limits=face_contrast_limits,
            z_index=z_index,}

instead of returning a list containing data of poped item

output = [data]

brisvag · 2025-07-04T08:34:50Z

Uh, I was thinking to only return the data itself (coordinates), but I guess we could do like you said and return a dict.

Another option is to split off the pop method into a new PR and then we can just get remove merged, which would be a nice benefit.

Yeah good idea! @rahul713rk do you think you could split this pop part out into a separate PR so we can discuss there, and in the meantime we can merge the remove/remove_selected changes?

rahul713rk · 2025-07-04T08:53:01Z

thanks @brisvag , sure going to remove pop part from this PR and opening a new issue on pop.

psobolewskiPhD · 2025-09-09T01:50:38Z

I pushed the requested change @brisvag from .sort() method to sorted
So now both points and shapes remove use sorted.

psobolewskiPhD · 2025-09-09T03:25:02Z

The windows test fails are unrelated, happening everywhere. Maybe from the windows runner changes.

version, checking selected_data after removal

psobolewskiPhD · 2025-09-12T03:26:58Z

@brisvag OK, i think selection works correctly now. it was actually tricky, because when you remove items from the set, the indices of remaining items change! So you need to update the indices that are in selected_data.

brisvag

Nice! Only thing is maybe to check for performance, but otherwise LGTM.

brisvag · 2025-09-12T12:11:25Z

+                for idx in self.selected_data:
+                    # If the selected index was removed, skip it
+                    if idx in indices:
+                        continue
+                    # Count how many indicies have been removed prior
+                    shift = bisect.bisect_left(indices, idx)
+                    new_selected.add(idx - shift)


Did you test with big arrays if this is slow?

so what's the worst case scenario here, it's large number of points and then selecting all of them?
Should probably check for that case and short circuit the loop.

But otherwise large number of points with most selected and some being deleted?

Not sure, I guess it depends on what's slower: the loop or calling bisect_left on a big array?

Ok so performance in the case of many selected points and removing some subset of points was not great.
The last two commits improve that case significantly, like 10X, by using sets better and using essentially the same approach as bisect, but numpy vectorized. (I got a little help from Gemini, testing it out.)

Removing selected shapes is horribly slow. I made an issue for that:

Removing selected shapes is very slow #8264
It requires a batch method on ShapesList or something -- outside the scope of this PR.

Note, it's technically not a regression, because on main you can't have some points selected and remove some other/overlaping set of points.
Removing all selected is not affected performance wise.

Removing all selected shapes is now marginally faster, but still horrendously slow once you get beyond 100s of shapes.

Nice, thanks for diving in!

brisvag · 2025-09-15T09:31:14Z

+                selected_not_removed = self.selected_data - set(indices)
+                if selected_not_removed:
+                    indices_array = np.array(indices)
+                    remaining_selected = np.array(list(selected_not_removed))


using np.fromiter seems almost 2X faster on my machine than passing through a list intermediate.

Yea using that is a bit faster. Like 10% or so. I will push the change when I get some free time.

Czaki · 2025-09-17T12:49:22Z

Hm. I think we should add benchmarks for removing shapes and points and run on this PR.

@brisvag @TimMonko @psobolewskiPhD Should I make a separate PR with benchmarks or expect contributor to do it here?

brisvag · 2025-09-17T14:13:46Z

I think it's best if you do it since yopu're most familiar; also I think @psobolewskiPhD mostly topok over this PR anyways :)

psobolewskiPhD · 2025-09-17T14:34:54Z

IMO deep optimization is out of scope here? I made an issue for the slow shape removal. It's super slow on main too.

brisvag · 2025-09-19T09:39:32Z

I think it's more whether we are introducting a regression. But yeah, if you think it's good on that, we can do the benchmark separately IMO 👍

psobolewskiPhD · 2025-09-20T01:29:48Z

I think it's more whether we are introducting a regression.

There is no regression, because prior to this PR you could only remove selected shapes/points and the code for that is unchanged.

What this adds is removing by index, which means you can potentially have a set of selected shapes/points and remove some other set of points/shapes. This case requires shifting the indexes of the selected points, which I implemented. There may be optimizations possible, by using more numpy set functions? However, to be honest, I suspect that most people will either use remove() and not have things selected or will select things and then remove_selected(). In both of these cases, the index shifting required to update the selection will not be used.

Removing shapes is just slow period, with or without this pr: #8264

brisvag

Alright, at last I think we're good to go and get this merged :) Probably immediatly followed up by #8070, once conflicts are solved!

Thanks @rahul713rk, sorry again for taking long on this :)

# References & Related Issues * Closes #8070 * Follow-up to: #7964 , #8031 --- # Description * Implemented a `pop` method for both the `Points` and `Shapes` layers, enabling removal and return of point/shape data at a specified index. * Added tests for `pop` method. --------- Co-authored-by: a <a> Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com> Co-authored-by: Lorenzo Gaifas <brisvag@gmail.com>

imagesc-bot · 2025-10-02T11:45:19Z

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/napari-v0-6-5-is-out/116715/1

a added 2 commits June 16, 2025 15:37

[Update] Added remove , remove_selected and pop functions in `s…

3b6115d

…hapes.py`

[Update] Added remove , remove_selected and pop in Points

c9199e1

rahul713rk requested a review from a team as a code owner June 16, 2025 10:43

rahul713rk changed the title ~~[Update] Added remove , 'remove_selected and pop` in Shapes and Points~~ [Update] Added remove , remove_selected and pop in Shapes and Points Jun 16, 2025

TimMonko previously requested changes Jun 20, 2025

View reviewed changes

Comment thread src/napari/layers/points/points.py

TimMonko added this to the 0.6.2 milestone Jun 20, 2025

Added Emit data event with ActionType.REMOVED to notify about cha…

a454457

…nges

TimMonko modified the milestones: 0.6.2, 0.6.3 Jun 29, 2025

[Added] Test cases for remove and pop in points and shapes

61bad3d

github-actions Bot added the tests Something related to our tests label Jul 1, 2025

Merge branch 'main' into issue7964

b253da8

psobolewskiPhD reviewed Jul 3, 2025

View reviewed changes

rahul713rk mentioned this pull request Jul 4, 2025

Implement pop method for Shapes and Points #8070

Closed

a and others added 2 commits July 4, 2025 14:36

remove pop method from both points and shapes layer

1719208

Merge branch 'main' into issue7964

ab2b1d5

brisvag requested changes Sep 9, 2025

View reviewed changes

Comment thread src/napari/layers/points/points.py Outdated

Comment thread src/napari/layers/points/points.py

psobolewskiPhD added 4 commits September 11, 2025 22:57

Update remove in Points to handle selection properly

6387e4c

Update test_removing_points to check selected_data shift

c367223

Ensure proper selection handling when removing, like Points

884d3bb

Update the Shapes test_removing_shapes with the same logic as the Points

201ab9e

version, checking selected_data after removal

brisvag approved these changes Sep 12, 2025

View reviewed changes

Update points remove() to be more performant

7bd3c39

psobolewskiPhD mentioned this pull request Sep 13, 2025

Removing selected shapes is very slow #8264

Closed

psobolewskiPhD and others added 2 commits September 12, 2025 21:49

Improve performance when removing points while points are selected

4e19073

Merge branch 'main' into issue7964

34b8e15

brisvag reviewed Sep 15, 2025

View reviewed changes

use fromiter per Lorenzo

712b5dc

brisvag approved these changes Sep 24, 2025

View reviewed changes

brisvag added the ready to merge Last chance for comments! Will be merged in ~24h label Sep 24, 2025

Merge branch 'main' into issue7964

7215479

brisvag merged commit 4fd9863 into napari:main Sep 26, 2025
59 of 60 checks passed

github-actions Bot removed the ready to merge Last chance for comments! Will be merged in ~24h label Sep 26, 2025

brisvag changed the title ~~[Update] Added remove and remove_selected in Shapes and Points~~ Add remove() and remove_selected() in Shapes and Points Sep 26, 2025

brisvag changed the title ~~Add remove() and remove_selected() in Shapes and Points~~ Add remove() in Shapes and Points Sep 26, 2025

brisvag added the highlight PR that should be mentioned in next release notes label Sep 26, 2025

Uh oh!

Uh oh!

Conversation

rahul713rk commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

References and relevant issues

Description

Uh oh!

psobolewskiPhD commented Jun 20, 2025

Uh oh!

Uh oh!

rahul713rk commented Jun 20, 2025

Uh oh!

psobolewskiPhD commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

psobolewskiPhD commented Jun 26, 2025

Uh oh!

TimMonko commented Jun 29, 2025

Uh oh!

rahul713rk commented Jul 1, 2025

Uh oh!

codecov Bot commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

psobolewskiPhD commented Jul 3, 2025

Uh oh!

brisvag commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

psobolewskiPhD commented Jul 4, 2025

Uh oh!

rahul713rk commented Jul 4, 2025

Uh oh!

brisvag commented Jul 4, 2025

Uh oh!

rahul713rk commented Jul 4, 2025

Uh oh!

psobolewskiPhD commented Sep 9, 2025

Uh oh!

psobolewskiPhD commented Sep 9, 2025

Uh oh!

Uh oh!

Uh oh!

psobolewskiPhD commented Sep 12, 2025

Uh oh!

brisvag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Czaki commented Sep 17, 2025

Uh oh!

brisvag commented Sep 17, 2025

Uh oh!

psobolewskiPhD commented Sep 17, 2025

Uh oh!

brisvag commented Sep 19, 2025

Uh oh!

rahul713rk commented Jun 16, 2025 •

edited

Loading

psobolewskiPhD commented Jun 26, 2025 •

edited

Loading

codecov Bot commented Jul 1, 2025 •

edited

Loading

brisvag commented Jul 3, 2025 •

edited

Loading