Add interactive masking tool #521

nvaytet · 2024-06-03T11:27:45Z

This is scipp/essreduce#30 moved to Scippneutron.

Masks can be drawn on 1d and 2d data (using mpltoolbox).
They can then be saved to file, using Scipp's hdf5 format.
Only rectangular and horizontal/vertical spans are supported for now.

jl-wynen

I have a lot of comments but they are mostly about small issues. It looks good overall!

jl-wynen · 2024-06-07T09:01:22Z

pyproject.toml

@@ -87,6 +88,7 @@ filterwarnings = [
  'ignore:Widget.widget_types is deprecated:DeprecationWarning',
  'ignore:Widget.widgets is deprecated:DeprecationWarning',
  'ignore:`load_nexus` is deprecated:UserWarning',
+  'ignore:Passing unrecognized arguments to super:DeprecationWarning',


Where does this come from?

This is also in Plopp. The warning pops up when using ipympl. I was never able to fix it.

jl-wynen · 2024-06-07T09:01:54Z

requirements/basetest.in

@@ -2,6 +2,7 @@
 # Do not make an environment from this file, use test.txt instead!

 hypothesis
+ipympl


Could you use something like plopp[interactive] instead?

I tried this now but I messes things up in the nightly dependencies, where we want to get plopp and mpltoolbox from @main... :-(

jl-wynen · 2024-06-07T09:03:46Z

tests/conftest.py

+
+@pytest.fixture()
+def _use_ipympl():
+    matplotlib.use('module://ipympl.backend_nbagg')


Does this only apply to a single test? It looks like it switches to the ipympl backend for all subsequent tests.

hmm I thought it was using ipympl for only a given test, but maybe it sets it for all?

Ok it is indeed setting it for all the other tests

I think I managed to fix this by switching to ipympl for the test and switching back to matplotlib defaults after the test.

jl-wynen · 2024-06-07T09:15:31Z

tests/masking_tool_test.py

+# Copyright (c) 2024 Scipp contributors (https://github.com/scipp)
+
+import json
+import tempfile


Please use pytest.tmp_path https://docs.pytest.org/en/6.2.x/tmpdir.html#the-tmp-path-fixture instead. This makes sure the files get cleaned up properly but still allows you to inspect them after the test ran.

jl-wynen · 2024-06-07T09:17:16Z

tests/masking_tool_test.py

Does it make sense to make the click API public? It is jot great to rely on implementation details in tests. And maybe, this could be useful for building UIs in the future where we may have some initial masks. But this may be too hypothetical...

Does it make sense to make the click API public? It is jot great to rely on implementation details in tests

Not sure; once we add it it's more difficult to hide/remove it again.
What would it look like? A .click(x, y) method which would look up which tool is curently active and forward to that tool?

And maybe, this could be useful for building UIs in the future where we may have some initial masks. But this may be too hypothetical...

I think that is an edge case far away in the future.

jl-wynen · 2024-06-07T09:40:48Z

docs/user-guide/masking-tool.ipynb

+    "h._tool.click(0, -20)\n",
+    "h._tool.click(0, -10)\n",
+    "\n",
+    "masking_tool.filename.value = \"masks-2d.h5\""


Suggested change

"masking_tool.filename.value = \"masks-2d.h5\""

"masking_tool.filename.value = \"masks-2d.json\""

jl-wynen · 2024-06-07T09:41:32Z

docs/user-guide/masking-tool.ipynb

+    "v._tool.click(20, 150)\n",
+    "v._tool.click(40, 150)\n",
+    "\n",
+    "masking_tool.filename.value = \"masks-1d.h5\""


Suggested change

"masking_tool.filename.value = \"masks-1d.h5\""

"masking_tool.filename.value = \"masks-1d.json\""

jl-wynen · 2024-06-07T09:42:48Z

docs/user-guide/masking-tool.ipynb

+   "metadata": {},
+   "outputs": [],
+   "source": [
+    "masking_tool"


Can you add an explanation for how to manipulate masks? I found myself clicking around a lot and accidentally creating a lot of masks.

I added instructions in the docstring and a help button that has a tooltip with instructions.
I unfortunately could not add just an icon with a tooltip, it had to be a button, but clicking the button does nothing (see jupyter-widgets/ipywidgets#3703 (comment)).

I experimented with having hidden instructions that would appear if you clicked the button but in the end, this was the simplest and didn't mess up the layout of the figure.

jl-wynen · 2024-06-07T09:43:58Z

docs/user-guide/masking-tool.ipynb

Is it possible to remove masks? It would not be great to have to start over every time you accidentally add a new mask (easy by misclicking).

Also, an undo button would go a long way. But that might be difficult and a task for another PR.

Is it possible to remove masks?

You can remove them by middle-clicking. I added this to the instructions.

Also, an undo button would go a long way. But that might be difficult and a task for another PR.

Yes, difficult and annoying. We'd have to keep a history etc...

jl-wynen · 2024-06-07T09:44:52Z

src/scippneutron/masking.py

+            disabled=ndim == 1,
+            **common,
+        )
+        self.controls = [rects, vspans, hspans]


These tools don't have tooltips.

nvaytet · 2024-06-07T11:44:35Z

.copier-answers.yml

@@ -5,7 +5,7 @@ description: Neutron scattering tools for Data Reduction
 max_python: '3.12'
 min_python: '3.10'
 namespace_package: ''
-nightly_deps: scipp,scippnexus,plopp
+nightly_deps: scipp,scippnexus,plopp,mpltoolbox


I had to edit this manually because it was needed for the nightly deps and I didn't want to do a copier update at the same time

jl-wynen · 2024-06-11T08:18:39Z

src/scippneutron/masking.py

+        • Left-click a vertex to edit a shape
+        • Right-click and hold to drag a shape
+        • Middle-click (or Ctrl + left-click) to delete a shape
+        • Save the masks to a file when the "Save" button is clicked


What are these bullet characters? Are they rendered properly by Sphinx?

I made bullets in the tooltip, and copy/pasted. I'll change to -.

jl-wynen · 2024-06-11T11:30:58Z

One last comment that may be hard to find in the list: #521 (comment)

add masking tool

1e14d7e

nvaytet marked this pull request as draft June 3, 2024 13:41

nvaytet added 6 commits June 3, 2024 16:54

add tests

7633d17

ruff

4139d9e

add ipympl dependency

d7dad8b

silence test warnings

9ffac5a

save masks as json instead of scipp hdf5

002d6a2

bump plopp version

58a4d5e

nvaytet marked this pull request as ready for review June 4, 2024 21:13

nvaytet and others added 3 commits June 4, 2024 23:25

add mpltoolbox to conda env for building docs

fd19fbc

add masking-tool to docs toc

017dd99

add tests for saving and loading masks from json

933ed3f

jl-wynen self-assigned this Jun 7, 2024

jl-wynen reviewed Jun 7, 2024

View reviewed changes

nvaytet added 6 commits June 7, 2024 11:59

Merge branch 'main' into masking-tool

fa46594

add mpltoolbox to nightly deps

32cc2fd

add instructions tooltip and allow overwriting mask files

6e479d7

better error message for higher dimension data

9dd95f3

add test for 3d data

f6f88d0

update notebook

821cc48

nvaytet commented Jun 7, 2024

View reviewed changes

use ipympl only for masking tool tests

107ea2b

jl-wynen reviewed Jun 11, 2024

View reviewed changes

replace bullets with -

0993c65

change format for masks info

263bc59

jl-wynen approved these changes Jun 11, 2024

View reviewed changes

nvaytet and others added 3 commits June 11, 2024 16:08

update figs before rendering

26869e2

Apply automatic formatting

17117c6

remove never execute that should not have been committed

8f038b0

nvaytet merged commit a6f8d2a into main Jun 11, 2024
4 checks passed

nvaytet deleted the masking-tool branch June 11, 2024 15:04

This was referenced Jun 19, 2024

[Requirement] Masking tools scipp/essdiffraction#50

Closed

Masking tools scipp/ess#176

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add interactive masking tool #521

Add interactive masking tool #521

nvaytet commented Jun 3, 2024

jl-wynen left a comment

jl-wynen Jun 7, 2024

nvaytet Jun 7, 2024

jl-wynen Jun 7, 2024

nvaytet Jun 7, 2024

jl-wynen Jun 7, 2024

nvaytet Jun 7, 2024

nvaytet Jun 7, 2024

nvaytet Jun 11, 2024

jl-wynen Jun 7, 2024

jl-wynen Jun 7, 2024

nvaytet Jun 7, 2024

jl-wynen Jun 7, 2024

jl-wynen Jun 7, 2024

jl-wynen Jun 7, 2024

nvaytet Jun 7, 2024 •

edited

Loading

jl-wynen Jun 7, 2024

nvaytet Jun 7, 2024

jl-wynen Jun 7, 2024

nvaytet Jun 7, 2024

jl-wynen Jun 11, 2024

nvaytet Jun 11, 2024

jl-wynen commented Jun 11, 2024

	"masking_tool.filename.value = \"masks-2d.h5\""
	"masking_tool.filename.value = \"masks-2d.json\""

	"masking_tool.filename.value = \"masks-1d.h5\""
	"masking_tool.filename.value = \"masks-1d.json\""

Add interactive masking tool #521

Add interactive masking tool #521

Conversation

nvaytet commented Jun 3, 2024

jl-wynen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvaytet Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jl-wynen commented Jun 11, 2024

nvaytet Jun 7, 2024 •

edited

Loading