Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add get_subsets_as_regions method to subset plugin, deprecate get_interactive_regions and get_spectral_regions #3340

Merged
merged 8 commits into from
Dec 16, 2024

Conversation

cshanahan1
Copy link
Contributor

@cshanahan1 cshanahan1 commented Dec 6, 2024

This PR adds get_regions to the Subset Tools plugin, and deprecates get_interactive_regions and get_spectral_regions.

@github-actions github-actions bot added cubeviz specviz imviz plugin Label for plugins common to multiple configurations labels Dec 6, 2024
@cshanahan1 cshanahan1 added this to the 4.1 milestone Dec 6, 2024
@cshanahan1
Copy link
Contributor Author

cshanahan1 commented Dec 9, 2024

I kept the scope in this ticket limited to consolidating functions that obtain subsets as regions all to the Subset Tools plugin in the new 'get_regions' method. There will be some follow up items to address:

First, two items that are part of the bigger picture:

  1. 'load_regions' and 'get_regions' should eventually round trip for every subset. This requires making a decision of how to treat Composite Subsets that are ouput as CompoundRegions. I think it would be easier to decide CompoundRegions should not be supported at all*, and have 'get_regions' output a list of individual Regions and a list of combination modes, which is what 'load_regions' currently supports. In this PR, I maintained the behavior and output format of 'get_interactive_regions' for spatial subsets, which returns a CompoundRegion for certain Composite Subsets. These of course will not round trip. I put in a test marked as xfail which should pass once this follow up is addressed.

  2. There are now only two places now to retrieve subsets as regions objects, as intended: App.get_subsets and Subset Tools.and get_regions. Subset Tools.and get_regions should use app.get_subsets behind the scenes to avoid code duplication, since the same info is available. Doing this requires deciding on an output format for Composite Subsets (do we support CompoundRegions or do we enforce a list of Subsets and Combination modes) so I can reformat the output of get_subsets to be regions-specific. (relevant to Concept notebooks for composite subset to scope out work #3320)

And second, some isolated follow up items:

  1. 'get_subsets', which will eventually be used by 'get_regions', should put in place a check if subsets can be combined into recognized shapes (e.g., two circular subsets applied with xor that share a center can be combined into a circular annulus).

  2. get_interactive_regions is broken when linked by sky, and since this PR uses that logic it is also broken. This might be fixed naturally when re-writing to use get_subsets.

Copy link

codecov bot commented Dec 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.70%. Comparing base (6c946d3) to head (3a49293).
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3340      +/-   ##
==========================================
- Coverage   88.80%   88.70%   -0.11%     
==========================================
  Files         125      125              
  Lines       19137    19206      +69     
==========================================
+ Hits        16995    17037      +42     
- Misses       2142     2169      +27     

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

Copy link
Member

@kecnry kecnry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thanks!

Copy link
Contributor

@javerbukh javerbukh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you!

@cshanahan1 cshanahan1 merged commit 1373414 into spacetelescope:main Dec 16, 2024
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cubeviz imviz plugin Label for plugins common to multiple configurations specviz
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants