Multi-image overlay #25

kaitj · 2024-09-09T14:22:04Z

Minor one here, following up on #21 - enables multi-image overlay capabilities.

Removes the 2 record limit prevoiusly placed on main image + overlay
Switches overlays and overlay colormaps from single input to a list of inputs, looping over them to generate the frame as needed. If more overlays than color maps provided, it just defaults to the same color map (alternatively, we could raise an exception?)

codecov · 2024-09-09T14:27:56Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.72%. Comparing base (3285dcd) to head (9569366).

Additional details and impacted files

@@           Coverage Diff           @@
##             main      #25   +/-   ##
=======================================
  Coverage   99.71%   99.72%           
=======================================
  Files          18       18           
  Lines         708      720   +12     
=======================================
+ Hits          706      718   +12     
  Misses          2        2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/niftyone/figures/generator.py

src/niclips/figures/multi_view.py

tests/unit/niclips/figures/test_multi_view.py

src/niclips/figures/multi_view.py

clane9 · 2024-09-10T14:50:03Z

src/niclips/figures/multi_view.py

A general comment on the approach in multi_view_frame for multiple overlays, which probably also applies elsewhere. Continuing to extend this function to support richer composite images might be adding some debt in the longer term. For example, you can feel some complexity in the logic for associating overlays and overlay colormaps. We might consider moving toward some way to compose images of multiple "layers" using multiple calls, as in eg matplotlib where multiple calls to plt.imshow stack on top of each other. (In fact, the function internally does some layer composition with noimg.overlay. So implementing this might not be too hard.)

Yeah I agree, though I don't anticipate overlays getting more complex than this. With the colormaps, I don't love handling it this way either. The idea of layering is sounds like a good approach to me. Would need to take a look into what this might look like.

Looked into this a little more - should be doable, though I might push this down the road and implement in a separate PR. Seems like that might be a bigger change and want to avoid this one getting too large + resolve multi-image overlays not working.

src/niclips/figures/multi_view.py

clane9 · 2024-09-10T14:59:18Z

src/niclips/figures/multi_view.py

    panel_height: int | None = 256,
    cmap: str = "gray",
-    overlay_cmap: str = "brg",
+    overlay_cmap: list[str] = ["brg"],


Why does the default overlay cmap change? I would guess it would be shared across functions, as in matplotlib with the default cmap.

If you're providing multiple overlays, you may not want to have the same cmap for them (e.g. multiple binary overlays of different tissue types). I don't think there are a lot of situations where you would want a different cmap, but the option is there if it is needed.

I just mean "brg" here vs "turbo" elsewhere. Very minor point though.

Ahh, I was actually going to ask you that 😅, those were the defaults from before.

Oh, so I did this 😅 😅? Ya I can't remember any reason for doing this.

Going to leave this as is for now unless you really feel strongly about it 😛

src/niclips/figures/multi_view.py

src/niftyone/figures/generator.py

src/niclips/figures/multi_view.py

kaitj · 2024-09-19T18:26:36Z

A few changes here. The main ones since last review:

Typecast view_kwargs to a MappingProxyType on ViewGenerator initialization. Will make it less likely to accidently mutate the kwargs once generator is initialized.
Passed the overlays directly to the view_fn in stead of mutating view_kwargs to do so.
Simplified generation of the figure output filepath (superceding Fix figure name mutation #29). Broke down the generate call to a number of smaller internal calls. Also rather than the hard-to-follow entities manipulation, this has been simplified in the intiialization of the figure_entities dict.
- Also didn't realize any non-standard entity is automatically put into extra_entities by b2t, so eliminated the need for setting it as a nested entity value.

Also addressed a number of the previous comments, except for:

Default colormaps being different.
Multiple overlays as layers - this is a bigger PR and will look to address this in a future PR.

~~One separate issue has arised stemmed from a recent pymongo update and is the cause of the now failing tests. This is addressed as a part of #30.~~
(Fixed in latest patch fix in FO)

- Also update tests - For videos, kept using a single overlay cmap as the mapping is done in a separate function, where as static images are not.

Instead of setting a single cmap and cycling through (which only really works for binary images), the user is to pass a list of color maps to use, one for each overlay. If there are more overlays than color maps, warn the user and use the previously set default of 'turbo'. This cause an issue if there are 2+ overlays don't have corresponding images - this will also serve as an additional warning for the user when they check the images.

kaitj · 2024-09-25T19:21:16Z

Merging this one with note (see below from offline discussion) to tackle some of challenges in a subsequent PR.

From @clane9

I think one source of issue is that the base class is trying to do a lot. all the special case logic for handling overlays and constructing file paths might be better handled in the relevant sub-classes, where the approach can also afford to be less general. For example, suppose you want a figure that overlays a tissue segmentation on a T1. You might want to incorporate some entities from the segmentation into the file name. But this is currently not possible bc the base class constructs file names based only on the first record (the T1).

* Add multi-overlay support in multi_view.py * Color maps need to be passed as a list matching the number of overlays provided, otherwise will be set to default. * Replace missed 'generators' variations * Simplify generation of output figure file path * Avoid having to set "extra_entities" key - b2t will automatically put these in there. Ensure staticmethods will accept additional kwargs. * Rather than manipulate entities before updating, create a new figure_entities that first updates the existing record entities with the figure entities of the generator. If "figure" is provided in the view_kwargs, update the figure entities with this.

* Add multi-overlay support in multi_view.py * Color maps need to be passed as a list matching the number of overlays provided, otherwise will be set to default. * Replace missed 'generators' variations

kaitj added the enhancement New feature or request label Sep 9, 2024

kaitj requested a review from clane9 September 9, 2024 14:22

kaitj self-assigned this Sep 9, 2024

clane9 reviewed Sep 9, 2024

View reviewed changes

src/niftyone/figures/generator.py Outdated Show resolved Hide resolved

src/niclips/figures/multi_view.py Show resolved Hide resolved

tests/unit/niclips/figures/test_multi_view.py Show resolved Hide resolved

kaitj force-pushed the maint/multi_image_overlay branch 2 times, most recently from 2285283 to 5bc3771 Compare September 10, 2024 02:16

kaitj requested a review from clane9 September 10, 2024 13:22

clane9 reviewed Sep 10, 2024

View reviewed changes

kaitj mentioned this pull request Sep 18, 2024

Fix figure name mutation #29

Closed

kaitj force-pushed the maint/multi_image_overlay branch 2 times, most recently from 5bb9787 to 2ca8e39 Compare September 19, 2024 18:14

kaitj requested a review from clane9 September 19, 2024 18:15

kaitj force-pushed the maint/multi_image_overlay branch 5 times, most recently from e495d43 to 4af812e Compare September 21, 2024 21:00

kaitj added 3 commits September 25, 2024 10:27

Add multi-overlay support in multi_view.py

af8d355

- Also update tests - For videos, kept using a single overlay cmap as the mapping is done in a separate function, where as static images are not.

Rm logic for disabling multi-image overlays

1e5eb8c

kaitj force-pushed the maint/multi_image_overlay branch from 4af812e to 80d691f Compare September 25, 2024 14:28

kaitj linked an issue Sep 25, 2024 that may be closed by this pull request

Default figure name not kept #27

Closed

Replace 'generators' variations

9569366

kaitj merged commit 7effae1 into childmindresearch:main Sep 25, 2024
5 checks passed

kaitj deleted the maint/multi_image_overlay branch September 25, 2024 19:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi-image overlay #25

Multi-image overlay #25

kaitj commented Sep 9, 2024

codecov bot commented Sep 9, 2024 •

edited

Loading

clane9 Sep 10, 2024

kaitj Sep 10, 2024

kaitj Sep 12, 2024

clane9 Sep 10, 2024

kaitj Sep 10, 2024

clane9 Sep 10, 2024

kaitj Sep 10, 2024

clane9 Sep 10, 2024

kaitj Sep 12, 2024

kaitj commented Sep 19, 2024 •

edited

Loading

kaitj commented Sep 25, 2024

Multi-image overlay #25

Multi-image overlay #25

Conversation

kaitj commented Sep 9, 2024

codecov bot commented Sep 9, 2024 • edited Loading

Codecov Report

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

kaitj commented Sep 19, 2024 • edited Loading

kaitj commented Sep 25, 2024

codecov bot commented Sep 9, 2024 •

edited

Loading

kaitj commented Sep 19, 2024 •

edited

Loading