Add Sky Regions to get_subsets() #2496

cshanahan1 · 2023-10-05T13:30:06Z

allow override of wcs when computing sky region in roi_subset_state_to_region glue-viz/glue-astronomy#96 (actually don't think this is needed after all)

~~Update glue-astronomy pin in pyproject.toml after the PR above is merged and released.~~

Description

Adds API to export sky regions for spatial subsets, as part of get_subsets(). If 'export_sky_region'=True, both a pixel and a sky region will be returned from get_subsets.

JDAT 3498

~~Requires glue-viz/glue-astronomy#96~~

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.
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

jdaviz/app.py

jdaviz/tests/test_subsets.py

pllim · 2023-10-05T23:59:05Z

Also need:

to fix the CI
add change log
decide if any new user doc is needed

cshanahan1 · 2023-10-06T19:40:46Z

@pllim @javerbukh i was going to add some documentation for this and i noticed the 'get_interactive_regions' function is how getting subsets back is advertised in the documentation. i think i should probably add a to_sky flag there as well, although that doesn't call 'get_subsets' (not sure why?) so i would need to put some logic in there too. thoughts on this? do we not advertise get_subsets but rather this function?

pllim · 2023-10-06T19:51:14Z

There is a conflict now. Please rebase. Thanks!

get_interactive_regions

I think @kecnry created a tech debt ticket on get_interactive_regions vs get_subsets. It is a battle for another day. Just ignore it unless you are very compelled to open that can of worms.

cshanahan1 · 2023-10-06T19:52:54Z

There is a conflict now. Please rebase. Thanks!

get_interactive_regions

I think @kecnry created a tech debt ticket on get_interactive_regions vs get_subsets. It is a battle for another day. Just ignore it unless you are very compelled to open that can of worms.

in that case, i will hold off on adding user documentation for this until that is decided.

pllim · 2023-10-07T01:45:19Z

CHANGES.rst

@@ -9,9 +9,13 @@ Cubeviz

 - Add circular annulus subset to toolbar. [#2438]

+- Expose sky regions in get_subsets. [#2496]


Expose how? A user needs to know enough to use it.

jdaviz/app.py

pllim · 2023-10-07T01:52:52Z

jdaviz/app.py

+        sky_subset_region = None
+        override_wcs = None  # only for cubeviz, where gwcs is missing spatial info
+        skip_sky_region = False
+        if to_sky:
+            wcs = subset_state.xatt.parent.coords
+            if self.config == 'cubeviz':
+                parent_data = subset_state.attributes[0].parent
+                override_wcs = parent_data.meta["_orig_spatial_wcs"]
+                if override_wcs is None:
+                    # if cubeviz and theres no spatial wcs, we have to skip computing sky region
+                    skip_sky_region = True
+            else:
+                if wcs is None:
+                    skip_sky_region = True  # also need to skip if in imviz and theres no WCS


I think this can be simplified (but please confirm that it actually works because other code below might also need changing to go with this):

Suggested change

sky_subset_region = None

override_wcs = None # only for cubeviz, where gwcs is missing spatial info

skip_sky_region = False

if to_sky:

wcs = subset_state.xatt.parent.coords

if self.config == 'cubeviz':

parent_data = subset_state.attributes[0].parent

override_wcs = parent_data.meta["_orig_spatial_wcs"]

if override_wcs is None:

# if cubeviz and theres no spatial wcs, we have to skip computing sky region

skip_sky_region = True

else:

if wcs is None:

skip_sky_region = True # also need to skip if in imviz and theres no WCS

if to_sky and self.config == 'cubeviz':

parent_data = subset_state.xatt.parent # Can you not use xatt in Cubeviz like in glue-astronomy? Not sure.

if "_orig_spec" in parent_data.meta:

to_sky = parent_data.meta["_orig_spec"].wcs

else:

to_sky = parent_data.coords

The point here is that you only need to hack around when it is Cubeviz and when the data has the "_orig_spec" hack. Otherwise, let existing backend handle things. Unless there is another use case that also needs hacking?

pllim · 2023-10-07T01:57:18Z

jdaviz/app.py

+        roi_as_sky_region = None
+        if to_sky and not skip_sky_region:
+            roi_as_sky_region = roi_subset_state_to_region(subset_state,
+                                                           to_sky=True,
+                                                           override_wcs=override_wcs)


Now that I think about this more, there is no point in calling roi_subset_state_to_region again. I overlooked the fact that you actually want to keep both pixel and sky regions. Once you have the pixel region, you only need one more step to convert it to sky region; you don't need glue-astronomy here.

Suggested change

roi_as_sky_region = None

if to_sky and not skip_sky_region:

roi_as_sky_region = roi_subset_state_to_region(subset_state,

to_sky=True,

override_wcs=override_wcs)

if to_sky:

roi_as_sky_region = roi_as_region.to_sky(to_sky)

else:

roi_as_sky_region = None

pllim · 2023-10-07T02:00:37Z

jdaviz/configs/cubeviz/plugins/parsers.py

+
+        # store original WCS in metadata. this is a hacky workaround for converting subsets
+        # to sky regions, where the parent data of the subset might have dropped spatial WCS info
+        metadata['_orig_spatial_wcs'] = wcs.celestial


I don't know how I feel about introducing yet one more hidden meta key to hack around Cubeviz WCS stuff. Why not use the existing "_orig_spec"? Does doing that introduce too big a performance penalty? This comment applies to all other similar occurrences.

Suggested change

metadata['_orig_spatial_wcs'] = wcs.celestial

metadata['_orig_spec'] = sc

we only sometimes need the full sc (only when theres a unit mismatch), but (for now) we always need the wcs (just the celestial component too) so i decided to always store that since its inexpensive. it will also be easier to refactor (rather than putting a check in if sc is not stored, then store wcs, then unpacking it later and doing the same check for sc)

pllim · 2023-10-10T18:30:13Z

I suggested changes to glue-viz/glue-astronomy#96 . If those are accepted, this PR needs to adapt.

jdaviz/tests/test_subsets.py

pllim · 2023-10-12T14:36:12Z

Timeout is from:

jdaviz/jdaviz/configs/imviz/tests/test_parser.py

Line 63 in 14b61e5

    
           self.jwst_asdf_url_2 = 'https://stsci.box.com/shared/static/d5k9z5j05dgfv6ljgie483w21kmpevni.fits'  # noqa: E501

https://stsci.box.com/shared/static/d5k9z5j05dgfv6ljgie483w21kmpevni.fits

Maybe Box is thinking we're spamming it. It still available.

codecov · 2023-10-12T14:56:25Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Files	Coverage Δ
jdaviz/app.py	`85.35% <100.00%> (+0.13%)`	⬆️
jdaviz/configs/cubeviz/plugins/parsers.py	`84.32% <93.75%> (+0.61%)`	⬆️

... and 1 file with indirect coverage changes

📢 Thoughts on this report? Let us know!.

javerbukh

This is definitely nice to have, good work! My only thought is that the sky region could be given automatically (without having to set include_sky_region to True), but I could see that causing performance issues so maybe we shelve that idea for a future date.

pllim · 2023-10-12T17:44:28Z

There is a change log conflict now. Can you please rebase? Thanks!

author Clare Shanahan <cshanahan@stsci.edu> 1696510996 -0400 committer Clare Shanahan <cshanahan@stsci.edu> 1696622631 -0400 adding sky regions to get_subsets

pllim

Very close! Some minor comments. Thanks!

If you run out of time, lemme know. Will be happy to help you wrap this up.

jdaviz/app.py

jdaviz/configs/cubeviz/plugins/parsers.py

pllim · 2023-10-12T19:47:17Z

jdaviz/configs/cubeviz/plugins/parsers.py

@@ -404,7 +444,9 @@ def _parse_ndarray(app, file_obj, data_label=None, data_type=None,

    if not hasattr(flux, 'unit'):
        flux = flux << u.count
-    s3d = Spectrum1D(flux=flux)
+
+    meta = standardize_metadata({'_orig_spatial_wcs': None})


I am not sure if standardize_metadata is needed for simple cases like this but I guess some consistency does not hurt.

jdaviz/tests/test_subsets.py

pllim · 2023-10-12T19:49:18Z

jdaviz/tests/test_subsets.py

+        wcs = spectral_cube_wcs.celestial
+        data = NDData(np.ones((128, 128)) * u.nJy, wcs=wcs)


Suggested change

wcs = spectral_cube_wcs.celestial

data = NDData(np.ones((128, 128)) * u.nJy, wcs=wcs)

data = NDData(np.ones((128, 128)) * u.nJy, wcs=imviz_2d_wcs)

You might have to update the results below.

same as my explanation above. i'll put a comment in the test to explain why im using the cube wcs here.

jdaviz/tests/test_subsets.py

pllim

Thanks!

* adding sky regions to get_subsets * final review comments * code style

cshanahan1 requested review from rosteen, javerbukh, pllim, kecnry, bmorris3 and haticekaratay as code owners October 5, 2023 13:30

github-actions bot added the testing label Oct 5, 2023

pllim added this to the 3.8 milestone Oct 5, 2023

pllim reviewed Oct 5, 2023

View reviewed changes

pllim added cubeviz imviz labels Oct 5, 2023

cshanahan1 mentioned this pull request Oct 6, 2023

allow override of wcs when computing sky region in roi_subset_state_to_region glue-viz/glue-astronomy#96

Closed

cshanahan1 force-pushed the export_sky_region branch from 6d1a207 to 215ccef Compare October 6, 2023 20:04

pllim reviewed Oct 7, 2023

View reviewed changes

pllim reviewed Oct 11, 2023

View reviewed changes

jdaviz/tests/test_subsets.py Outdated Show resolved Hide resolved

javerbukh approved these changes Oct 12, 2023

View reviewed changes

cshanahan1 added 2 commits October 12, 2023 13:59

parent 9dfab3a

c7fac91

author Clare Shanahan <cshanahan@stsci.edu> 1696510996 -0400 committer Clare Shanahan <cshanahan@stsci.edu> 1696622631 -0400 adding sky regions to get_subsets

.

6fe09b7

cshanahan1 force-pushed the export_sky_region branch from 17ebb44 to 6fe09b7 Compare October 12, 2023 18:01

pllim reviewed Oct 12, 2023

View reviewed changes

cshanahan1 added 2 commits October 12, 2023 16:03

final review comments

5663bc6

code style

e772e7c

.

78e7297

pllim approved these changes Oct 12, 2023

View reviewed changes

pllim enabled auto-merge (squash) October 12, 2023 20:27

pllim merged commit 62c6233 into spacetelescope:main Oct 12, 2023
11 of 12 checks passed

pllim mentioned this pull request Oct 16, 2023

Rehome subsets after removing data from viewer. #2409

Merged

rosteen pushed a commit to rosteen/jdaviz that referenced this pull request Oct 19, 2023

Add Sky Regions to get_subsets() (spacetelescope#2496)

865d2fc

* adding sky regions to get_subsets * final review comments * code style

pllim mentioned this pull request Nov 17, 2023

get_subsets doesn't return sky_region for composite subset created with 'remove'[BUG] #2570

Open

pllim mentioned this pull request May 1, 2024

Fix logic flow in _parse_hdulist for Cubeviz #2842

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Sky Regions to get_subsets() #2496

Add Sky Regions to get_subsets() #2496

cshanahan1 commented Oct 5, 2023 •

edited

Loading

pllim commented Oct 5, 2023

cshanahan1 commented Oct 6, 2023 •

edited

Loading

pllim commented Oct 6, 2023

cshanahan1 commented Oct 6, 2023

pllim Oct 7, 2023

pllim Oct 7, 2023

pllim Oct 7, 2023

pllim Oct 7, 2023

cshanahan1 Oct 11, 2023 •

edited

Loading

pllim commented Oct 10, 2023

pllim commented Oct 12, 2023 •

edited

Loading

codecov bot commented Oct 12, 2023 •

edited

Loading

javerbukh left a comment

pllim commented Oct 12, 2023

pllim left a comment

pllim Oct 12, 2023

pllim Oct 12, 2023

pllim Oct 12, 2023

cshanahan1 Oct 12, 2023

pllim left a comment

		@@ -9,9 +9,13 @@ Cubeviz

		- Add circular annulus subset to toolbar. [#2438]

		- Expose sky regions in get_subsets. [#2496]

	metadata['_orig_spatial_wcs'] = wcs.celestial
	metadata['_orig_spec'] = sc

		wcs = spectral_cube_wcs.celestial
		data = NDData(np.ones((128, 128)) * u.nJy, wcs=wcs)

Add Sky Regions to get_subsets() #2496

Add Sky Regions to get_subsets() #2496

Conversation

cshanahan1 commented Oct 5, 2023 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

pllim commented Oct 5, 2023

cshanahan1 commented Oct 6, 2023 • edited Loading

pllim commented Oct 6, 2023

cshanahan1 commented Oct 6, 2023

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

cshanahan1 Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

pllim commented Oct 10, 2023

pllim commented Oct 12, 2023 • edited Loading

codecov bot commented Oct 12, 2023 • edited Loading

Codecov Report

javerbukh left a comment

Choose a reason for hiding this comment

pllim commented Oct 12, 2023

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

pllim left a comment

Choose a reason for hiding this comment

cshanahan1 commented Oct 5, 2023 •

edited

Loading

cshanahan1 commented Oct 6, 2023 •

edited

Loading

cshanahan1 Oct 11, 2023 •

edited

Loading

pllim commented Oct 12, 2023 •

edited

Loading

codecov bot commented Oct 12, 2023 •

edited

Loading