Create import_region method for loading region objects #3104

javerbukh · 2024-07-19T14:59:44Z

Description

This pull request creates an import_region method that loads a region object with combination_mode.
Some todo:

Implement loading region file using

jdaviz/jdaviz/core/helpers.py

Line 651 in dfb4bfa

def load_regions_from_file(self, region_file, region_format='ds9', max_num_regions=20,
self.combination_mode should use the Select (SubsetSelect?) class to be able to take advantage of the choices and selected attributes
Connect self.combination_mode to the subset mode selection in the toolbar
Add tests
Add documentation for new API

Fixes #

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)?

codecov · 2024-07-19T15:23:39Z

Codecov Report

Attention: Patch coverage is 95.27559% with 6 lines in your changes missing coverage. Please review.

Project coverage is 88.45%. Comparing base (88ff8da) to head (7610f50).
Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
...igs/default/plugins/subset_plugin/subset_plugin.py	95.12%	6 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #3104    +/-   ##
========================================
  Coverage   88.45%   88.45%            
========================================
  Files         124      124            
  Lines       18389    18649   +260     
========================================
+ Hits        16266    16496   +230     
- Misses       2123     2153    +30

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

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py

pllim

All the existing tests using apply_roi should be updated to use this new API too?

Is there any benefit to use locally scoped imports vs importing everything you need at the top of the module?

Other comments as follow.

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py

pllim · 2024-08-13T18:32:43Z

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py

+            self._import_spectral_regions(regions, mode)
+            return


Given that you return here, all the spatial region imports (if you still want them locally scoped) should happen after this.

You should return bad_regions here to be consistent with spatial loader behavior, which means your _import_spectral_regions should give you back bad_regions like the spatial one does.

Suggested change

self._import_spectral_regions(regions, mode)

return

return self._import_spectral_regions(regions, mode)

pllim · 2024-08-13T18:33:50Z

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py

+        self.app.hub.broadcast(SnackbarMessage(
+            f"Loaded {n_loaded}/{n_reg_in} regions, max_num_regions={max_num_regions}, "
+            f"bad={n_reg_bad}", color=snack_color, timeout=8000, sender=self.app))
+        if return_bad_regions:
+            return bad_regions


This behavior should be emulated by the spectral region loader too, to be consistent.

jdaviz/configs/default/plugins/subset_plugin/tests/test_subset_plugin.py

jdaviz/configs/imviz/tests/test_regions.py

jdaviz/core/helpers.py

notebooks/CubevizExample.ipynb

notebooks/SpecvizExample.ipynb

kecnry · 2024-08-13T18:50:45Z

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py

+    def _combination_selected_updated(self, change):
+        self.app.session.edit_subset_mode.mode = SUBSET_MODES[self.combination_mode.selected]
+
+    def _update_combination_mode(self):


this is only ever called when linking is changed, but needs to be two-way synced, so that changing the mode in the UI (the glue dropdown) or via internal glue API, is reflected in self.combination_mode

has this been addressed? When changing the combination mode from the UI, I do not see the internal traitlet being updated to match (although changing the combination_mode.selected does reflect in the UI via _combination_selected_updated).

javerbukh · 2024-08-14T17:42:41Z

@pllim @kecnry I believe I have addressed all review comments except the ones about _import_spectral_regions behaving exactly like the spatial subset creation. I need to put this PR down until the 26th so please feel free to handle that additional work however you see fit.

pllim · 2024-08-14T19:38:21Z

I need to put this PR down until the 26th

And this blocks 4.0?

kecnry · 2024-08-15T12:19:55Z

And this blocks 4.0?

This is not listed as required for 4.0. In fact, if we do merge it, we may want to consider postponing exposing the API so that we continue to iterate on the public API during the follow-up efforts.

pllim · 2024-08-15T13:07:50Z

I think it is important for the API to have consistent behavior if we want the same thing to handle both spatial and spectral. So if we want to wait till after 4.0 anyway, then maybe this can wait for Jesse to come back?

bmorris3

This looks good. Should we also change the example notebooks (ImvizExample, CubevizExample, ImvizDitheredExample, imviz_advanced_aper_phot, imviz_color_mpl, imviz_edit_subset_programmatic, cubeviz-cropped) which still have calls to helper.load_regions?

Stale

kecnry · 2024-08-27T15:19:53Z

Now that the notebooks are updated, that would make this official public API (we can no longer just defer exposing the API), so do we want to milestone this beyond 4.0?

pllim · 2024-08-27T15:51:39Z

I thought we already said this PR is for after 4.0, no?

camipacifici · 2024-09-12T20:20:24Z

Thank you for the new updates!
I leave here something for discussion. My workflow is in specviz where I want to create three subsets
region = SpectralRegion(1*u.um, 2*u.um) #alone
region2 = SpectralRegion(3*u.um, 4*u.um) #alone
regionlist = SpectralRegion([(1.5*u.um, 2.5*u.um),(3.5*u.um, 4.5*u.um)]) #2 wavelength ranges in same subset
This does the job:
subtools._obj.import_region(region, combination_mode='new')
subtools._obj.import_region(region2, combination_mode='new')
subtools._obj.import_region(regionlist, combination_mode=['new','or'])
I wonder if combination_mode='new' should be the default for the first subset, to be able to achieve the same thing with this instead
subtools._obj.import_region(region)
subtools._obj.import_region(region2)
subtools._obj.import_region(regionlist, combination_mode='or')
What do people think?

Setting subtools._obj.combination_mode = 'new' at the beginning gives me a long traceback at the first import_region, but maybe I am doing something wrong there.

javerbukh · 2024-09-13T12:19:14Z

The first import_region call should create a new subset by default and all following calls will need the combination_mode='new' parameter. The reason for that is our viewer.apply_roi API behaves the same as the GUI where every call after the first just updates the selected subset. Creating a new subset with each call is what helper.load_regions does. This method can do both but the default glue and apply_roi implementation is also the default here.

kecnry · 2024-09-13T12:33:51Z

Setting subtools._obj.combination_mode = 'new' at the beginning gives me a long traceback at the first import_region

Do we understand the cause for this? Maybe it can be a separate effort to fix if it wasn't introduced here, but might make this type of workflow simpler if its supported?

javerbukh · 2024-09-13T13:24:25Z

Sorry, forgot to address this point! You need to do subtools._obj.combination_mode.selected = 'new' because combination_mode is a SelectPluginComponent object. You can also do combination_mode.choices to see options for what to select.

kecnry · 2024-09-13T13:46:41Z

ah ok, good to know its only that!. (But note that once exposed to the user API, the .selected will no longer be required)

pllim

I think we're at the point where this has dragged on long enough that it should just go in so we can move on to the follow-up tickets. CI is green, so approving. Thanks for your patience and persistence!

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py

kecnry · 2024-09-17T17:13:20Z

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py

+    def _combination_selected_updated(self, change):
+        self.app.session.edit_subset_mode.mode = SUBSET_MODES[self.combination_mode.selected]
+
+    def _update_combination_mode(self):


has this been addressed? When changing the combination mode from the UI, I do not see the internal traitlet being updated to match (although changing the combination_mode.selected does reflect in the UI via _combination_selected_updated).

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py

kecnry · 2024-09-17T17:17:14Z

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py

+        else:
+            data = self.app.data_collection[refdata_label]
+
+        # TODO: Make this work for data cube.


is this a new TODO comment or copied from the previous region import code?

Copied from previous, I'll remove it.

does it work for cubes? If not, let's make sure we have a ticket (but safe to ignore here if this was from moved code)

Seems like this works, although the solution is hacky (per glue-viz/glue-astronomy#75 ), maybe @pllim can clarify on the status. Subset creation works with cubes so I think this is safe to remove but I'll wait for Pey-Lian's answer.

@rosteen reworked some spectrum WCS stuff recently so I'll defer to him. Or was it @dhomeier ? As long as the tests don't fail and results don't change, whatever works is fine by me.

If it works it works 🤷 I don't recall changing the spectrum WCS stuff recently in a way that would affect this.

That issue has been closed by glue-viz/glue-astronomy#75 (comment), and the translator would only use a PaddedSpectrumWCS if the cube had a 1D WCS, which I assume is not the case here.

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py

kecnry · 2024-09-17T17:20:14Z

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py

+        combination_mode : list, str, or `None`
+            The way that regions are created or combined. If a list, then it must be the
+            same length as regions. If `None`, then it will follow the default glue
+            functionality for subset creation.


could/should we restore the original combination mode after this either completes or raises an exception?

My thought would be the combination mode stays the last entry in the list but I am interested to hear what would make the most sense for a user.

ok, let's discuss and decide with the team. Whatever the decision, its probably worth explaining here in the API docs so the users know what to expect. 🐱

javerbukh · 2024-09-18T18:22:47Z

@kecnry Waiting to hear from @rosteen on whether the line about checking a cube's wcs is still valid but otherwise I addressed the review comments. I tested changing the edit mode using the UI and API and it seemed to update correctly. Please let me know if you have a workflow I can use to test that behavior.

As for the comment on combination_mode reverting to default after import_region is called, I think we should defer that discussion to a follow-up. I created ticket for that in the subset plugin epic on Jira.

kecnry · 2024-09-23T13:43:39Z

I tested changing the edit mode using the UI and API and it seemed to update correctly. Please let me know if you have a workflow I can use to test that behavior.

Screen.Recording.2024-09-23.at.9.42.42.AM.mov

kecnry · 2024-09-23T18:36:21Z

After digging into this more, this might just be a limitation of how glue handles the mode dropdown vs the actual mode that will be used (when there is no active subset selected, there will be a new created subset regardless of the mode dropdown). If/when we want to expose a UI element in the plugin, we'll definitely need to think about this a little more. I'm not sure if/how we should deal with this for the exposed API though 🤔

kecnry

LGTM now - any remaining items can be iterated as we build on top of this before we make the API exposed as public. Thanks for working through all these iterations!

If we want this merged before @javerbukh gets back, I think we could just accept the few small code suggestions I listed and then merge.

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py

kecnry · 2024-09-27T17:19:38Z

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py

+            Default is to load everything.  If you are providing a large file/list
+            input for ``region``, it is recommended
+
+        refdata_label : str or `None`


I wonder if we could just do data_label - but we can revise this anytime before making public, so can ignore for now.

kecnry · 2024-09-27T17:21:35Z

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py

+                try:
+                    raw_regs = Regions.read(region, format=region_format)
+                except Exception:  # nosec
+                    raw_regs = SpectralRegion.read(region)


This is fine for now, but I wonder if there's any way we could predict in advance instead of try/except. As it is now, checking the extension would probably work, but in the future we may have the same extension supported between spectral + spatial, so that isn't a longterm solution.

pllim · 2024-09-27T20:46:52Z

we could just accept the few small code suggestions I listed and then merge.

+1

javerbukh added this to the 4.0 milestone Jul 19, 2024

github-actions bot added the plugin Label for plugins common to multiple configurations label Jul 19, 2024

javerbukh force-pushed the import-region-subset-plugin branch 2 times, most recently from fe49ace to cae3fa1 Compare August 12, 2024 17:55

javerbukh marked this pull request as ready for review August 12, 2024 19:59

javerbukh requested review from rosteen, pllim, kecnry, bmorris3, haticekaratay, cshanahan1 and gibsongreen as code owners August 12, 2024 19:59

kecnry reviewed Aug 13, 2024

View reviewed changes

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py Outdated Show resolved Hide resolved

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py Outdated Show resolved Hide resolved

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py Outdated Show resolved Hide resolved

pllim previously requested changes Aug 13, 2024

View reviewed changes

This comment was marked as resolved.

Sign in to view

kecnry reviewed Aug 13, 2024

View reviewed changes

javerbukh marked this pull request as draft August 13, 2024 21:08

javerbukh marked this pull request as ready for review August 14, 2024 17:39

javerbukh requested review from pllim and kecnry August 14, 2024 17:42

bmorris3 reviewed Aug 16, 2024

View reviewed changes

javerbukh force-pushed the import-region-subset-plugin branch from 1557dad to 288e0f3 Compare August 27, 2024 12:32

javerbukh added 6 commits September 10, 2024 16:21

Fix code style

acfff62

Fix docstring error

4d02cd8

Change create_new_after to create_as_new

42f0409

Address review comments

964903a

Fix code style

e00818f

Fix test failure with rebase

f9ac560

javerbukh force-pushed the import-region-subset-plugin branch from 127df8d to f9ac560 Compare September 10, 2024 20:28

javerbukh added 6 commits September 11, 2024 10:34

Update tests to use subset_plugin._obj

ff256a3

Fix code style

d53e3de

Fix import region call in data quality test

4e3d3cb

Fix indentation in docs

b661711

Replace mode and create_as_new with combination_mode

a1b6f59

Fix code style

138c27c

pllim approved these changes Sep 16, 2024

View reviewed changes

kecnry reviewed Sep 17, 2024

View reviewed changes

Address comments

3ab4e03

pllim mentioned this pull request Sep 24, 2024

BUG: Fix wrong Subset unit when created in 2D spectrum viewer #3201

Merged

9 tasks

kecnry approved these changes Sep 27, 2024

View reviewed changes

Apply suggestions from code review

7610f50

kecnry force-pushed the import-region-subset-plugin branch from 7218aa8 to 7610f50 Compare September 30, 2024 11:44

kecnry merged commit f48bc16 into spacetelescope:main Sep 30, 2024
19 checks passed

	self._import_spectral_regions(regions, mode)
	return
	return self._import_spectral_regions(regions, mode)

Create import_region method for loading region objects #3104

Create import_region method for loading region objects #3104

Conversation

javerbukh commented Jul 19, 2024 • edited by pllim Loading

Description

Change log entry

Checklist for package maintainer(s)

codecov bot commented Jul 19, 2024 • edited Loading

Codecov Report

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

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javerbukh commented Aug 14, 2024

pllim commented Aug 14, 2024

kecnry commented Aug 15, 2024

pllim commented Aug 15, 2024

bmorris3 left a comment

Choose a reason for hiding this comment

kecnry commented Aug 27, 2024

pllim commented Aug 27, 2024

camipacifici commented Sep 12, 2024

javerbukh commented Sep 13, 2024

kecnry commented Sep 13, 2024

javerbukh commented Sep 13, 2024

kecnry commented Sep 13, 2024

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

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

kecnry Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

javerbukh commented Sep 18, 2024

kecnry commented Sep 23, 2024

kecnry commented Sep 23, 2024

kecnry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim commented Sep 27, 2024

javerbukh commented Jul 19, 2024 •

edited by pllim

Loading

codecov bot commented Jul 19, 2024 •

edited

Loading

kecnry Sep 18, 2024 •

edited

Loading