Refactor get subsets to work with composite subset state #2087
Conversation
There was a problem hiding this comment.
I need to give more thought to the actual behavior and the change in output-format between specviz and cubeviz, for example.... but just wanted to get a few thoughts down on my first pass.
This case seems incorrect to me if I'm understanding the intention of the output... without any concept of nesting, I'd expect the top two entries at the end of the video to both show as InvertState, is that right?
Screen.Recording.2023-03-16.at.5.29.24.PM.mov
|
@kecnry Good find, I think I need to use AndNotState instead of InvertState since the first is more accurate but because of glue reasons that was not as easy to do. I'll give that a try now though. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2087 +/- ##
==========================================
+ Coverage 91.90% 91.93% +0.03%
==========================================
Files 143 143
Lines 15605 15789 +184
==========================================
+ Hits 14342 14516 +174
- Misses 1263 1273 +10
... and 4 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
jdaviz/app.py
Outdated
|
|
||
| dc = self.data_collection | ||
| subsets = dc.subset_groups | ||
| # TODO: Use global display units |
|
@pllim I thought we agreed to write changelogs after 2 approvals? I was hoping to add documentation as a follow-up to this PR |
jdaviz/configs/specviz/plugins/line_analysis/tests/test_line_analysis.py
Outdated
Show resolved
Hide resolved
You could, yes. I just did not see any mention of that plan in your post. Thanks for the clarification! |
javerbukh
force-pushed
the
refactor-get-subsets
branch
2 times, most recently
from
2048ac4 to
8b3db42
Compare
javerbukh
requested review from
duytnguyendtn,
rosteen,
bmorris3 and
haticekaratay
as code owners
Add get_subsets method which replaces get_subsets_from_viewer Remove unused imports Fix specviz helper tests Fix failing tests and address review comments Address review comments Ignore ipykernel deprecation warning Add tests covering composite spectral regions Add check if subset label not in dc subset groups Rename to object_only
javerbukh
force-pushed
the
refactor-get-subsets
branch
from
00de06d to
339c75a
Compare
There was a problem hiding this comment.
This seems to fix a few bugs and adds support for accessing nested spatial subsets. Since this is an app-level method, I think we can defer discussions/debate about API-specifics until/unless we implement a user-friendly helper-level method (I'm not sure spectral/spatial/object only options are intuitive for a user, but seem to do the trick for internal calls we need for now).
Edit: CI will need to be passing, but I'm approving now since I'll be out for a bit - if fixing CI results in significant changes, feel free to mark the approval as stale.
| @@ -246,12 +307,13 @@ def test_get_spectral_regions_unit_conversion(specviz_helper, spectrum1d): | |||
|
|
|||
| specviz_helper.app.get_viewer("spectrum-viewer").apply_roi(XRangeROI(0.6, 0.7)) | |||
|
|
|||
| # TODO: Is this test still relevant with the upcoming glue unit conversion changes? | |||
There was a problem hiding this comment.
should we remove this entirely? Or would re-enabling it break?
There was a problem hiding this comment.
I think we should wait until the glue unit conversion changes are implemented in jdaviz to decide what to do with this code block.
There was a problem hiding this comment.
but how should it be expected to behave now? The subsets still have .lower.unit attributes, right?
There was a problem hiding this comment.
The spectral subsets, yes.
There was a problem hiding this comment.
This test uses conv_func = uc.UnitConversion.process_unit_conversion so should it be considered relevant at all?
jdaviz/app.py
Outdated
| subsets = dc.subset_groups | ||
| # TODO: Use global display units | ||
| # units = dc[0].data.coords.spectral_axis.unit | ||
| viewer = self.get_viewer("spectrum-viewer") |
There was a problem hiding this comment.
This line is giving me error in Imviz when I have Subsets and I call imviz.app.get_subsets(). There is no doc updates to go with this, so I am not sure how I am supposed to test this in Imviz. Please advise. thanks!
File jdaviz/app.py:858, in Application.get_subsets(self, subset_name, spectral_only, spatial_only, object_only)
855 subsets = dc.subset_groups
856 # TODO: Use global display units
857 # units = dc[0].data.coords.spectral_axis.unit
--> 858 viewer = self.get_viewer("spectrum-viewer")
859 data = viewer.data()
860 # Set axes labels for the spectrum viewer
File jdaviz/app.py:589, in Application.get_viewer(self, viewer_reference)
564 def get_viewer(self, viewer_reference):
565 """
566 Return a `~glue_jupyter.bqplot.common.BqplotBaseView` viewer instance.
567 This is *not* an ``IPyWidget``. This is stored here because
(...)
587 The viewer class instance.
588 """
--> 589 return self._viewer_by_reference(viewer_reference)
File jdaviz/app.py:1409, in Application._viewer_by_reference(self, reference)
1394 """
1395 Viewer instance by reference defined in the yaml configuration file.
1396
(...)
1405 The viewer class instance.
1406 """
1407 viewer_item = self._viewer_item_by_reference(reference)
-> 1409 return self._viewer_store[viewer_item['id']]
TypeError: 'NoneType' object is not subscriptable
There was a problem hiding this comment.
p.s. Didn't @bmorris3 have things like this to prevent hardcoding "spectrum-viewer"?
jdaviz/jdaviz/configs/cubeviz/helper.py
Line 20 in 5510486
There was a problem hiding this comment.
Yes but this was supposed to be a temporary code block until the unit conversion refactor was implemented. I have a quick fix for this (and yes, a regression test 😈 ).
There was a problem hiding this comment.
Fix + test is in.
| @@ -389,3 +389,29 @@ def test_composite_region_with_consecutive_and_not_states(cubeviz_helper): | |||
|
|
|||
| spatial_list = cubeviz_helper.app.get_subsets("Subset 1", spatial_only=True) | |||
| assert len(spatial_list) == 3 | |||
|
|
|||
|
|
|||
| def test_composite_region_with_imviz(imviz_helper, image_2d_wcs): | |||
Co-authored-by: P. L. Lim <[email protected]>
|
In the Imviz example notebook, after you run the part where you add more stuff as Subsets via API, you'll get this:
Now, when you use an existing API to grab them, it will know to ignore the MaskedSubset because we don't know how to handle arbitrary masks that cannot translate to a proper >>> imviz.get_interactive_regions()
{'Subset 1': <CirclePixelRegion(center=PixCoord(x=1578.3658447265625, y=1079.412109375), radius=12.68408203125)>,
'Subset 2': <CirclePixelRegion(center=PixCoord(x=600.0, y=400.0), radius=10.0)>,
'Subset 3': <CirclePixelRegion(center=PixCoord(x=1379.402894779631, y=467.8100724131408), radius=20.729537026656292)>,
'Subset 4': <CirclePixelRegion(center=PixCoord(x=600, y=200), radius=20)>,
'Subset 5': <CirclePixelRegion(center=PixCoord(x=1379.402894779631, y=467.8100724131408), radius=41.459074053312584)>}But with your new API, it is not filtered out. >>> imviz.app.get_subsets()
{'Subset 1': [{'name': 'CircularROI',
'glue_state': 'RoiSubsetState',
'region': <CirclePixelRegion(center=PixCoord(x=1578.3658447265625, y=1079.412109375), radius=12.68408203125)>}],
'Subset 2': [{'name': 'CircularROI',
'glue_state': 'RoiSubsetState',
'region': <CirclePixelRegion(center=PixCoord(x=600.0, y=400.0), radius=10.0)>}],
'Subset 3': [{'name': 'CircularROI',
'glue_state': 'RoiSubsetState',
'region': <CirclePixelRegion(center=PixCoord(x=1379.402894779631, y=467.8100724131408), radius=20.729537026656292)>}],
'Subset 4': [{'name': 'CircularROI',
'glue_state': 'RoiSubsetState',
'region': <CirclePixelRegion(center=PixCoord(x=600, y=200), radius=20)>}],
'Subset 5': [{'name': 'CircularROI',
'glue_state': 'RoiSubsetState',
'region': <CirclePixelRegion(center=PixCoord(x=1379.402894779631, y=467.8100724131408), radius=41.459074053312584)>}],
'MaskedSubset 1': None}And if I start to pass non-default options to your API, it crashes: >>> imviz.app.get_subsets(spatial_only=True, object_only=True)
jdaviz/app.py:878, in Application.get_subsets(self, subset_name, spectral_only, spatial_only, object_only)
876 elif spatial_only and not isinstance(subset_region, SpectralRegion):
877 if object_only:
--> 878 all_subsets[label] = [reg['region'] for reg in subset_region]
879 else:
880 all_subsets[label] = subset_region
TypeError: 'NoneType' object is not iterable |
|
Now, if I go back to Subset1 and make it a compound subset (i.e., use the "add" and then add a Rectangle), I get this with existing API (TBH I am surprised it even work at all!): >>> imviz.get_interactive_regions()
{'Subset 1': <CompoundPixelRegion(region1=Region: RectanglePixelRegion
center: PixCoord(x=1564.6979764661519, y=1089.153806782053)
width: 13.82388050415193
height: 13.047258003918841
angle: 0.0 rad, region2=Region: CirclePixelRegion
center: PixCoord(x=1578.3658447265625, y=1079.412109375)
radius: 12.68408203125, operator=<built-in function or_>)>,
...}I think it is consistent with your API: >>> imviz.app.get_subsets(subset_name="Subset 1")
[{'name': 'RectangularROI',
'glue_state': 'OrState',
'region': <RectanglePixelRegion(center=PixCoord(x=1564.6979764661519, y=1089.153806782053), width=13.82388050415193, height=13.047258003918841, angle=0.0 deg)>},
{'name': 'CircularROI',
'glue_state': 'RoiSubsetState',
'region': <CirclePixelRegion(center=PixCoord(x=1578.3658447265625, y=1079.412109375), radius=12.68408203125)>}]But again your API crashes when I pass in one more option: >>> imviz.app.get_subsets(subset_name="Subset 1", object_only=True)
jdaviz/app.py:883, in Application.get_subsets(self, subset_name, spectral_only, spatial_only, object_only)
881 elif not spectral_only and not spatial_only:
882 if object_only and not isinstance(subset_region, SpectralRegion):
--> 883 all_subsets[label] = [reg['region'] for reg in subset_region]
884 else:
885 all_subsets[label] = subset_region
TypeError: 'NoneType' object is not iterable |
|
Does not seem to affect Aperture Photometry plugin, and no Imviz tests have changed. 👏 |
|
@pllim I have that case fixed now. I had not run into |
|
@javerbukh , it doesn't crash but when I have masked subset, the output of |
and both look fine to me. Did you do something else in your workflow? I had no idea we used |
|
I say, you should not even include it in your output. I don't see any point because when I do |
|
Follow-up ticket for this work is #2114. |
Description
This pull request is to address a few issues:
The work that still needs to happen:
[{"name": <RegionClassName>, "glue-state": <AndState/InvertState/OrState/ROIState>, "region": <PixelRegion object>}]Change log entry
CHANGES.rst? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rstbefore merge. If no, maintainershould add a
no-changelog-entry-neededlabel.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.
triviallabel.