Adds multiple simultaneous slicers support #3758

jellybean2004 · 2025-11-14T13:34:17Z

Description

Implemented support for multiple simultaneous slicers on 2D plots, allowing users to create, manage, and switch between different slicer instances.

A unique ID is assigned to each slicer instance so they can be viewed simultaneously on the 2D fitting plot, and each generates an independent plot.
A list of all active slicers is added in the Slicer Parameters window so users can select and tweak them individually. The selected slicer gets automatically updated when a slicer is added or modified in the 2D plot. Users can also delete individual slicers from there.
Slicers have different colours to help differentiate them.

How Has This Been Tested?

Functionality tested in the program, works as expected.

Review Checklist:

[if using the editor, use [x] in place of [ ] to check a box]

Documentation (check at least one)

There is nothing that needs documenting
Documentation changes are in this PR
There is an issue open for the documentation (link?)

Installers

There is a chance this will affect the installers, if so
- Windows installer (GH artifact) has been tested (installed and worked)
- MacOSX installer (GH artifact) has been tested (installed and worked)
- Wheels installer (GH artifact) has been tested (installed and worked)

Licensing (untick if necessary)

The introduced changes comply with SasView license (BSD 3-Clause)

krzywon · 2025-11-14T21:59:38Z

I'm testing this locally and think this is looking incredibly promising! I can easily add any number of slicers to a 2D plot.

Questions and notes I have:

Should multiple slicers be added together, treated separately, or should there be a user option to switch? In the current implementation, they are all treated separately.
Is it possible to add a list of active slicers to the context menu, similar to how 1D plots list individual plots? This would help enable features (including ones I'm suggesting below), like removing a single slicer, changing the color of the slicer, etc.
Is it possible to remove only one of the slicers? This would allow a user to compare two slices, and only keep the one they want without having to keep another slicer visible.
Previous versions replaced the existing slicer when a new slicer was selected. This changes that paradigm. Is this desired, or should we include a way for the user to either add or replace a slicer?
Selecting a slicer on the plots highlights that slicer in the slicer parameters window, which is a nice feature!
Multiple, overlapping slicers are difficult to differentiate. Is it possible to make them all different colors or even give the user an option for colors?

jellybean2004 · 2025-11-15T10:01:46Z

I'm testing this locally and think this is looking incredibly promising! I can easily add any number of slicers to a 2D plot.

Questions and notes I have:

Should multiple slicers be added together, treated separately, or should there be a user option to switch? In the current implementation, they are all treated separately.

Is it possible to add a list of active slicers to the context menu, similar to how 1D plots list individual plots? This would help enable features (including ones I'm suggesting below), like removing a single slicer, changing the color of the slicer, etc.

Is it possible to remove only one of the slicers? This would allow a user to compare two slices, and only keep the one they want without having to keep another slicer visible.

Previous versions replaced the existing slicer when a new slicer was selected. This changes that paradigm. Is this desired, or should we include a way for the user to either add or replace a slicer?

Selecting a slicer on the plots highlights that slicer in the slicer parameters window, which is a nice feature!

Multiple, overlapping slicers are difficult to differentiate. Is it possible to make them all different colors or even give the user an option for colors?

Hi @krzywon! Welcome back, and thank you for your comments.

So this is the first step in the whole project I have at hand, which is just to enable the addition of multiple slicers at once, bringing it closer to the features available in Grasp. The next step is to plot them on the same plot. Finally, to link the slicers so that cases like peaks symmetrically distributed around the centre can be handled more easily.

I've just pushed a commit that implements deleting a single slicer.

Regarding the list of active slicers, I have updated the Slicer Parameters window to include a list of all the slicers shown in the plot. There, you can also check a particular slicer to tweak the parameters, and with the new commit, you can delete just the selected slicer too. Is this what you mean?

I could add a checkbox somewhere so users can either have the newly added multi-slicers or keep the previously supported single slicer, if that is something useful to keep.

You did raise a good point about the slicers being the same colour, and therefore being hard to differentiate (I was ignoring that, but I guess I have been caught 😅) I will give a shot at implementing that.

rozyczko

A few minor comments to the source and questions about implementation.

src/sas/qtgui/Plotting/Slicers/BaseInteractor.py

src/sas/qtgui/Plotting/Slicers/AnnulusSlicer.py

src/sas/qtgui/Plotting/Slicers/SectorSlicer.py

src/sas/qtgui/Plotting/SlicerParameters.py

jellybean2004 · 2025-11-16T10:32:23Z

@krzywon, I have changed the slicer colours to be different, so it is easier to differentiate. I have also added the functionality of individual slicer deletion, which can be done from the slicer parameter window.

@rozyczko, I have implemented most of your suggestions. I will now work on refactoring out the ID generation, as it is the same for each slicer.

Please let me know if there is anything else I could add, or something I missed.

…for any such evenys

butlerpd

Nice work though I have not looked closely at the code yet. This is mainly a functionality review based on window 11 testing of the installer.

Clear slicers does not clear the box sum box on the plot so once a box sum is slicer is activated, the box on the 2D plot is permanent and we have to close the whole plot before another slicer can be used cleanly. Not sure if this predates this PR in which case another issue might be appropriate, but prior to this PR a new slicer removed the old anyway?
There are really irritating white lines that randomly appear (apparently when another window passes over the plot?) but this seem to be there from before this pull request? In which case an issue might be the better place to worry about this.
I like that all slicers, even the annulus (and box sum though IMO we should eventually remove that as a thing) allows for multiple slicers. The color coding helps, but it is a bit unintuitive that the new slicer falls directly on top of the old one. To the newbie it looks like nothing happened? maybe just offset the interactors by a small fraction from the first one each time. If the third is on top of the second it won't matter as by that point it should be clear? and most people will move the second to where they want it before asking for a third slicer?
Alternatively I guess the offset could increment each time? This is more complicated for the annulus which does not have a center that ties the two sides together. Not sure how to deal with that, but I think the colloid orientation people will use the multiple annulus a lot.
Related, the top color is the color for the last slice. However, if one grabs the center point and moves the ROI, it is the very first one that gets moved. This is very counterintuitive. At first I thought that resizing the one I moved was not working as I assumed it should be updating the last 1D plot (the one on top).
Clearing slicers does not clear the 1D plots. Should it? I think so, specially now that we may have several plots from one 2D plot. Also @ChristianWoegerbauer work to promote the slicer plot to a regular data set, if the user wants to maitain that data they presumably will have pressed send to fitting?
It is not clear to me that there is a use case for having two different types of slicers on the plot at once? And if the normal desire is to change slicer types it might be useful to revert that behavior to clearing the slicers and creating the new one. That said, all the user has to learn to do is to clear slicers before asking for a different slicer type so maybe it is OK to leave as is for now?

butlerpd · 2025-11-17T14:42:39Z

Another question: what happens if I add enough slicers to go through the list of 15 defined colors? do the colors recycle back at the beginning?

jellybean2004 · 2025-11-17T14:48:59Z

Another question: what happens if I add enough slicers to go through the list of 15 defined colors? do the colors recycle back at the beginning?

Yes, the colours cycle back, but hopefully no one is adding more than 15 slicers 😅

krzywon

Adding a BoxSum deletes all other slicers, except existing BoxSum slicers, but previous BoxSum slicer interactors are no longer draggable on the plot. Clear Slicers does not remove any BoxSum, including the one the is still interactive.

Otherwise, see code comments.

Thanks for making the individual slicers removable and for the color work!

src/sas/qtgui/Plotting/Plotter2D.py

krzywon · 2025-11-17T14:05:50Z

src/sas/qtgui/Plotting/Plotter2D.py

+        # TODO: generalize this and put it in GuiUtils so that we can use it elsewhere
+        tempPlotsToRemove = []
+        slicer_type_id = 'Slicer' + self.data0.name
+        for itemIndex in range(item.rowCount()):


I'm almost certain this could be simplified into a single loop and a more compact if tree. CodeScene will gate on something like this, which is why I am noting it. I'll try to find a cleaner solution today.

krzywon · 2025-11-17T14:11:20Z

src/sas/qtgui/Plotting/Plotter2D.py

+                        self.slicer_widget.checkSlicerByName(name)
+                    break
+        except Exception:
+            pass


What exceptions could get a user here? Swallowing exceptions like this seems unsafe.

krzywon · 2025-11-17T14:45:40Z

src/sas/qtgui/Plotting/SlicerParameters.py

+
+    def onSlicerReplaceChanged(self, index):
+        """replace the slicer with the one chosen"""
+        if index == 0:  # No interactor


Are there any cases where index might be a non-integer value, like False, None, or empty strings? If not, this is fine, but if so, those Falsey values will fall through.

Side note: Type hints on this method would help clarify this situation.

krzywon · 2025-11-17T14:48:15Z

src/sas/qtgui/Plotting/SlicerParameters.py

+            try:
+                slicer_obj.clear()
+            except (ValueError, AttributeError):
+                pass


Were you seeing these errors? In what cases? Why are you swallowing them?

krzywon · 2025-11-17T14:49:16Z

src/sas/qtgui/Plotting/Slicers/AnnulusSlicer.py

+        # new_plot.name = "AnnulusPhi" + "(" + self.data.name + ")"
+        # new_plot.title = "AnnulusPhi" + "(" + self.data.name + ")"


Another instance of code commented out that should probably be removed instead.

krzywon · 2025-11-17T14:52:48Z

src/sas/qtgui/Plotting/Slicers/SlicerUtils.py

+Utility functions for slicers
+"""
+
+def generate_unique_plot_id(base_id, item):


Type hints, please.

krzywon · 2025-11-17T14:59:24Z

src/sas/qtgui/Plotting/Slicers/SlicerUtils.py

+    parent_item = item if item.parent() is None else item.parent()
+
+    existing = 0
+    for i in range(parent_item.rowCount()):


Are we assuming this tree only has a single set of children? That might change in the future.

Perhaps an intermediate, recursive method that returns the count for each level with this for loop would be better.

Suggested change

for i in range(parent_item.rowCount()):

def _row_count_for_level(parent_item, count: int):

for i in range(parent_item.rowCount()):

it = parent_item.child(i)

d = it.data()

if hasattr(d, "id") and isinstance(d.id, str) and d.id.startswith(base_id):

count += 1

if it.rowCount() > 0:

count += _row_count_for_level(it, count)

return count

butlerpd

having looked at the code a bit and the other comments I think this is essentially ready to go after fixing the issues named or pushing them to new issues for a future PR. Probably don't want this PR to grow too much?

krzywon · 2025-11-19T14:25:27Z

@butlerpd and @jellybean2004 - The box sum issues I've mentioned are new and should be addressed before I will approve. The one that is required is the box sum not being cleared when slicers are cleared so multiple box sum images end up on the plot. The box sum clearing all other slicers when a new box sum is generated is how it currently works, but is also different than the new functionality, so I'm conflicted on this.

rozyczko reviewed Nov 15, 2025

View reviewed changes

jellybean2004 added 21 commits November 17, 2025 12:52

Reformats slicer files

f1e7831

Enables multiple slicers on the same plot simultaneously

37a8351

Creates a new plot for a new slicer of the same type

beaada9

Fixes bug not properly removing slicers, and adds exception handling …

54bc9ec

…for any such evenys

Patches slicer issues

29fb7d1

Adds slicer list and selection in params window

87e35b6

Auto selection of current slider in slicers list

15b1d1f

Updates docs

74e9166

Implements single slicer delete button in slicer param window

2c733c0

Differentiate slicers with (colour-blind friendly!) colours

ca79d2d

Fixes create new slicer button

313c4c4

Enables replacement of slicers

77aaa27

Changes slicer list to radio buttons

80266a2

Removes unrequried try excepts

daa8a4d

Moves Delete button in Slicer Params to Slicers list

a11dfc4

Deletion bug fix

44ba688

Proper implementation of individual deletion

69a61d8

Check implementation from comments

da0e9e9

Patch for box deletion issues

2243ec2

Refactor out the unique plot ID generation for slicers

56b2e80

Combobox goes back to None after slicer is created from SP window

ff94b84

jellybean2004 force-pushed the multi_slicer_feature branch from 66d67ec to ff94b84 Compare November 17, 2025 12:04

Updates docs

0d7d807

butlerpd reviewed Nov 17, 2025

View reviewed changes

krzywon requested changes Nov 17, 2025

View reviewed changes

butlerpd approved these changes Nov 17, 2025

View reviewed changes

		# new_plot.name = "AnnulusPhi" + "(" + self.data.name + ")"
		# new_plot.title = "AnnulusPhi" + "(" + self.data.name + ")"

-    for i in range(parent_item.rowCount()):
+def _row_count_for_level(parent_item, count: int):
+    for i in range(parent_item.rowCount()):
+        it = parent_item.child(i)
+        d = it.data()
+        if hasattr(d, "id") and isinstance(d.id, str) and d.id.startswith(base_id):
+            count += 1
+        if it.rowCount() > 0:
+            count += _row_count_for_level(it, count)
+    return count

Adds multiple simultaneous slicers support #3758

Are you sure you want to change the base?

Adds multiple simultaneous slicers support #3758

Uh oh!

Conversation

jellybean2004 commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How Has This Been Tested?

Review Checklist:

Uh oh!

krzywon commented Nov 14, 2025

Uh oh!

jellybean2004 commented Nov 15, 2025

Uh oh!

rozyczko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jellybean2004 commented Nov 16, 2025

Uh oh!

butlerpd left a comment

Choose a reason for hiding this comment

Uh oh!

butlerpd commented Nov 17, 2025

Uh oh!

jellybean2004 commented Nov 17, 2025

Uh oh!

krzywon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

krzywon Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

krzywon Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

krzywon Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

krzywon Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

krzywon Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

krzywon Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

krzywon Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

butlerpd left a comment

Choose a reason for hiding this comment

Uh oh!

krzywon commented Nov 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jellybean2004 commented Nov 14, 2025 •

edited

Loading