slice: refactor plugin to decouple from cube #2715

kecnry · 2024-02-19T19:37:32Z

Description

This pull request refactors the slice plugin (as a follow-up to #2706) to decouple the slice logic from the cube (so that the slice plugin works with either NO cubes or eventually multiple cubes) and further generalization to disconnect the logic from assumptions about using profile viewers or a spectral axis.

IMPORTANT: This effectively removes exposing and allowing to set a slice by index.

allow slice viewer tool to become default when first becoming visible
allow downstream to override slice axis at the viewer-level
allow for multiple viewers with indicators by using mixins for WithSliceIndicator or WithSliceSelection
decouple slice axis from cube and replace with optional (enabled by default, forced to be enabled in cubeviz) "snap to slice" functionality
uses scientific notation on the indicator label only if necessary

Screen.Recording.2024-02-20.at.2.10.18.PM.mov

Change log entry

Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
should add a no-changelog-entry-needed label.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
Do the proposed changes follow the STScI Style Guides?
Are tests added/updated as required? If so, do they follow the STScI Style Guides?
Are docs added/updated as required? If so, do they follow the STScI Style Guides?
Did the CI pass? If not, are the failures related?
Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone. Bugfix milestone also needs an accompanying backport label.
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

pllim

Still unclear to me how this would work with Baby Shark (and cube without calibration wavelength axis in general).

pllim · 2024-02-24T02:25:34Z

jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.py

@@ -305,6 +304,11 @@ def _aperture_selected_changed(self, event={}):
        else:
            self._background_selected_changed()

+    @property
+    def _cubeviz_slice_ind(self):
+        fv = self.app.get_viewer(self.app._jdaviz_helper._default_flux_viewer_reference_name)


I don't know how this would be affected in the future when you allow multiple flux cubes and so on, but I guess we can worry about it in the future.

Agreed - for now I was just trying to get the previous behavior in aperture photometry working with the new design. This PR does not allow for multiple cubes, it just helps remove some assumptions in case we want to open that up later.

kecnry · 2024-02-26T12:48:03Z

Still unclear to me how this would work with Baby Shark (and cube without calibration wavelength axis in general).

I have not tested this yet (the PR is still in draft - I'm working to fix existing test cases first), but the slicing axis must be in some units, even if it is pixels or dimensionless. So I don't see any design issues that would conflict with this generality - we may just need to make sure the empty unit is handled correctly in the UI and indicator label.

codecov · 2024-03-04T15:31:31Z

Codecov Report

Attention: Patch coverage is 81.55340% with 57 lines in your changes are missing coverage. Please review.

Project coverage is 88.72%. Comparing base (af8d1b8) to head (d5a732f).
Report is 9 commits behind head on main.

❗ Current head d5a732f differs from pull request most recent head 2eea290. Consider uploading reports for the commit 2eea290 to get more accurate results

Files	Patch %	Lines
jdaviz/configs/cubeviz/plugins/slice/slice.py	73.74%	47 Missing ⚠️
jdaviz/core/marks.py	91.11%	4 Missing ⚠️
jdaviz/configs/cubeviz/plugins/viewers.py	94.64%	3 Missing ⚠️
...configs/default/plugins/export_plot/export_plot.py	50.00%	2 Missing ⚠️
jdaviz/configs/cubeviz/helper.py	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2715      +/-   ##
==========================================
- Coverage   88.85%   88.72%   -0.13%     
==========================================
  Files         108      108              
  Lines       15920    16138     +218     
==========================================
+ Hits        14145    14319     +174     
- Misses       1775     1819      +44

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

javerbukh · 2024-03-06T14:05:31Z

jdaviz/configs/cubeviz/plugins/slice/tests/test_slice.py

    sl = Slice(app=app)
+#    sl = cubeviz_helper.plugins['Slice']._obj


Which one of these makes more sense to use here?

I can remove the commented line if we want... I wanted to test to make sure it works both ways. I think in the future we may want to run all tests both ways since they do go through different logic routes... but that's a story for another day.

javerbukh · 2024-03-06T14:07:39Z

jdaviz/configs/cubeviz/plugins/slice/slice.py

-    min_slice = Int(0).tag(sync=True)
-    max_slice = Int(100).tag(sync=True)
-    wait = Int(200).tag(sync=True)
+    slider_throttle = Int(200).tag(sync=True)  # milliseconds


I noticed that clicking and dragging the slice indicator leaves it a fraction of a second behind my cursor, does that come from this throttle? Is there a particular reason 200 ms was chosen instead of something smaller?

This throttle was already there (I just renamed it from wait). This definitely will add a floor to the lag that is seen, but if the lag increases beyond this, we definitely should try to optimize the code so that it is more responsive. Is this PR noticeably less responsive than main for you?

No it's the same as main. Agreed on your point about coming back to this later and trying to optimize the code.

javerbukh

I left a couple comments but overall the code looks very clean and the slice plugin runs without any issues, nice work! Looking forward to seeing the multiple WithSliceIndicator viewers in LCViz and how that functionality is handled there.

kecnry · 2024-03-06T15:14:43Z

Looking forward to seeing the multiple WithSliceIndicator viewers in LCViz and how that functionality is handled there.

Feel free to look at spacetelescope/lcviz#85!

pllim · 2024-03-06T23:33:04Z

jdaviz/configs/cubeviz/plugins/slice/tests/test_slice.py

    sl = Slice(app=app)
+#    sl = cubeviz_helper.plugins['Slice']._obj


Shouldn't we prefer cubeviz_helper.plugins['Slice'] since that is the API we are advertising?

Suggested change

sl = Slice(app=app)

# sl = cubeviz_helper.plugins['Slice']._obj

sl = cubeviz_helper.plugins['Slice']._obj

One is instantiated at app startup and the other when called. So they run through different logic routes, see my reply above. We really should test both for all cases, but that would take some work. Here I ultimately don't care, I tested both by toggling the comment and it works.

But CI cannot do manual toggle like you did. If you want to test them both, then please use pytest.mark.parametrize. Thanks!

ok, I'll just remove the comment then and make a tech debt ticket 🐱 to consider parameterizing over this in all tests. I just had this for convenience to test myself.

pllim

Running through concepts/cubeviz_ndarray_gif.ipynb, the behavior isn't quite right. The concepts/cubeviz_wfc3ir_ramp.ipynb has all the problems described in "From array".

From array

X-axis label is called Pixel Axis 2 [x]; a little ugly but I can live with that. However, the Slice plugin says "Wavelength" that is clearly wrong. The play/pause buttons are all broken. Spectrum viewer mouseover is also broken.

Sometimes (seems non-deterministic), the text box under "Wavelength" seems to flash quickly to some float value before "snapping" to an integer index value. For example, I would see something like 2.0344243242 flashed real quick before it settles to 2. Not a show stopper though.

From GIF

Play/pause buttons work but mouseover on spectrum viewer still broken as above.

More shark!

At this point, the play/pause buttons stopped working (after I ran slice_plg.value = 0) but I am not sure why. Four of the buttons are disabled when they are not supposed to (accidentally flipped state?) and the play does not do anything.

Conclusion

Our regression test for "raw numpy cube" is too simple to catch all the subtle problems I saw above.

jdaviz/jdaviz/configs/cubeviz/plugins/tests/test_parsers.py

Line 183 in c731eef

def test_numpy_cube(cubeviz_helper):

This PR may have made things better for lcviz but it made Cubeviz much less flexible. With this in place, re-using Cubeviz for ramp visualization would be a bit harder because we have to go back and fix the things mentioned above.

If you insist that these cases only exists in "concepts" and so not supported, then I rather we just officially drop support than to have them still load but not work as well as before.

* but keep support for downstream

to support wave/wavelength/freq/frequency/wavenumber/velocity/energy

* can always change to be more intelligent based on data and x-display-unit later

pllim · 2024-03-08T19:01:12Z

Failure seems related:

        # Make sure nothing crashes if plugin used without data]
        sl.vue_play_next()
        sl.vue_play_start_stop()
>       assert not sl.is_playing
E       AssertionError: assert not True
...
slice/tests/test_slice.py::test_slice - AssertionError: assert not True
 +  where True = Slice(config='cubeviz', events=[
'create_cube_viewer', 'goto_first', 'goto_last', 'play_next', 'play_prev', 'play_start...le);\n
},\n  }\n</script>\n\n<style>\n  .v-slider {\n    margin: 0px !important;\n  }\n</style>\n'),
vdocs='latest').is_playing

(this would have quite immediately after otherwise, but is hard to reliably test since that is threaded)

kecnry · 2024-03-08T19:12:00Z

hmmm, I think that was just because the check was in the thread before and sometimes returned in time to be caught by the assert, but apparently not always. I now duplicated the check before the thread starts so hopefully the test is more reliable 🤞

[ci skip] [rtd skip]

pllim

I pushed a commit to update baby shark concept notebook but there is no need to rerun CI. I will merge now. Thanks!

from PR spacetelescope#2715

kecnry added this to the 3.9 milestone Feb 19, 2024

github-actions bot added cubeviz imviz plugin Label for plugins common to multiple configurations labels Feb 19, 2024

kecnry mentioned this pull request Feb 19, 2024

Time Selector (adapted version of cubeviz's slice) plugin spacetelescope/lcviz#85

Merged

kecnry force-pushed the slice-refactor branch 7 times, most recently from 6f24ff3 to 13170b8 Compare February 23, 2024 18:46

pllim reviewed Feb 24, 2024

View reviewed changes

kecnry force-pushed the slice-refactor branch from 13170b8 to 06ca234 Compare March 1, 2024 18:17

kecnry marked this pull request as ready for review March 5, 2024 16:07

kecnry requested review from rosteen, javerbukh, bmorris3, haticekaratay, cshanahan1 and gibsongreen as code owners March 5, 2024 16:07

javerbukh reviewed Mar 6, 2024

View reviewed changes

javerbukh approved these changes Mar 6, 2024

View reviewed changes

kecnry added the Ready for final review label Mar 6, 2024

pllim reviewed Mar 6, 2024

View reviewed changes

pllim requested changes Mar 7, 2024

View reviewed changes

kecnry added 16 commits March 8, 2024 10:04

support test data

c1c9504

deprecate cubeviz_helper.select_slice

b4d1fd4

edge checks and wrapping support in cube play functionality

3498272

update tests

306498b

reproduce previous logic for init slice

2d70a46

do not allow disabling snapping in cubeviz

a1ad818

* but keep support for downstream

avoid calls to viewer.data()

6d61574

ap phot: determine slice-index on the fly instead of relying on plugin

1546332

update even more tests

59becf7

read slice component from x_att

470a9b7

to support wave/wavelength/freq/frequency/wavenumber/velocity/energy

handle cubes passing int to slice value

38de9f8

further support for flexible x_att handling

0768998

more test updates

44ef041

fix support for slice axis given in integers

d1fe419

remove comment

a6e1ea4

fix play support when slices are integers

6e48798

kecnry force-pushed the slice-refactor branch from ca39d92 to 3d581b8 Compare March 8, 2024 15:04

allow initializing when slice axis is pixel

98f98f2

kecnry force-pushed the slice-refactor branch from 3d581b8 to 98f98f2 Compare March 8, 2024 15:07

always label as "value" in UI

2ec4624

* can always change to be more intelligent based on data and x-display-unit later

kecnry requested a review from pllim March 8, 2024 16:52

don't allow starting playing if no slices

d5a732f

(this would have quite immediately after otherwise, but is hard to reliably test since that is threaded)

DOC: Use new API in concept notebook

2eea290

[ci skip] [rtd skip]

pllim approved these changes Mar 8, 2024

View reviewed changes

pllim merged commit b4cdd08 into spacetelescope:main Mar 8, 2024
2 checks passed

kecnry deleted the slice-refactor branch March 8, 2024 20:09

kecnry added a commit to kecnry/jdaviz that referenced this pull request Mar 8, 2024

manually port changed logic from slice plugin refactor to new plugin

3f5a0d7

from PR spacetelescope#2715

kecnry mentioned this pull request Mar 8, 2024

Export plugin (deprecates/replaces "Export Plot") #2722

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

slice: refactor plugin to decouple from cube #2715

slice: refactor plugin to decouple from cube #2715

kecnry commented Feb 19, 2024 •

edited

Loading

pllim left a comment

pllim Feb 24, 2024

kecnry Feb 26, 2024

kecnry commented Feb 26, 2024 •

edited

Loading

codecov bot commented Mar 4, 2024 •

edited

Loading

javerbukh Mar 6, 2024

kecnry Mar 6, 2024

javerbukh Mar 6, 2024

kecnry Mar 6, 2024

javerbukh Mar 6, 2024

javerbukh left a comment

kecnry commented Mar 6, 2024

pllim Mar 6, 2024

kecnry Mar 6, 2024

pllim Mar 7, 2024

kecnry Mar 7, 2024 •

edited

Loading

pllim left a comment

pllim commented Mar 8, 2024 •

edited

Loading

kecnry commented Mar 8, 2024

pllim left a comment

		sl = Slice(app=app)
		# sl = cubeviz_helper.plugins['Slice']._obj

	sl = Slice(app=app)
	# sl = cubeviz_helper.plugins['Slice']._obj
	sl = cubeviz_helper.plugins['Slice']._obj

slice: refactor plugin to decouple from cube #2715

slice: refactor plugin to decouple from cube #2715

Conversation

kecnry commented Feb 19, 2024 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

pllim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kecnry commented Feb 26, 2024 • edited Loading

codecov bot commented Mar 4, 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

javerbukh left a comment

Choose a reason for hiding this comment

kecnry commented Mar 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kecnry Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

pllim left a comment

Choose a reason for hiding this comment

From array

From GIF

More shark!

Conclusion

pllim commented Mar 8, 2024 • edited Loading

kecnry commented Mar 8, 2024

pllim left a comment

Choose a reason for hiding this comment

kecnry commented Feb 19, 2024 •

edited

Loading

kecnry commented Feb 26, 2024 •

edited

Loading

codecov bot commented Mar 4, 2024 •

edited

Loading

kecnry Mar 7, 2024 •

edited

Loading

pllim commented Mar 8, 2024 •

edited

Loading