Concept notebooks for composite subset to scope out work #3320

pllim · 2024-11-25T23:11:53Z

Description

This pull request is to address, "Investigate: test existing composite subset support and scope additional work."

Spatial subsets
Spectral subsets

TODO for reviewers

Look at the two concept notebooks I added in this PR.
Discuss my recommendations in each.
Create follow-up tickets as needed.
You can close this PR without merge or merge it. I have no preference.
Mark https://jira.stsci.edu/browse/JDAT-4573 as Done.

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-11-26T23:17:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.80%. Comparing base (6c946d3) to head (89cffa5).
Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3320      +/-   ##
==========================================
- Coverage   88.80%   88.80%   -0.01%     
==========================================
  Files         125      125              
  Lines       19137    19137              
==========================================
- Hits        16995    16994       -1     
- Misses       2142     2143       +1

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

javerbukh · 2024-12-06T17:18:02Z

Thank you for investigating this so thoroughly! My understanding of the priorities for the points you bring up:

The top priority is making it clear to users what import_region can and cannot do with some combination of NotImplemented errors, documentation, and instructions in the UI.
Investigate using compound region for round tripping composite subsets.
Create a compound region-like thing for SpectralRegion objects.

I think some of the things you mentioned in the concept notebooks can be a part of those top 3 (for example, only allowing combination_mode = "or" for a filename would be part of 1, same with one file only being able to store one subset). Please let me know if I understood things correctly!

pllim · 2024-12-09T15:07:59Z

(2) Depends on how thorough a roundtrip you really want. If user does not mind the simplified version, then we're basically done for spectral regions.

(3) Depends on (2).

camipacifici · 2024-12-09T15:29:27Z

Sorry I am late here. I am ok with the recommendations in the imviz concept notebook. Will be looking at the specviz one shortly

camipacifici · 2024-12-09T20:01:50Z

Comments about the specviz notebook, assuming that one day we will be able to save multiple subsets in the same file.
I see a few use cases here:

user who wants to save many single subsets and reload them at once with combination 'new'. They are all separated anyway.
user who wants to save many single subsets and reload them at once. They will remember which ones need to be combined together and can do that with a separate method after loading, e.g., load subset 1 and subset 2 and then combine them with 'or' to make a new subset 3.
user who does not want to remember how they have combined subsets. Currently, they can save a single composite subset out and reload it with combination 'or' and everything will be fine. If they want to save all the subsets together, maybe the csv file could carry the information about the combination? Something like:
subset_name xmin xmax other subset combination_mode
I see this can be pretty hacky though. Is this what could be fixed by building the equivalent of compound_region in specutils?

Regardless, I think that the default when reading should be 'new', not 'replace'.

cshanahan1

(Some relevant thoughts to this notebook while working on subset api stuff):

See my comment in #3340. The load_regions and get_regions in subset tools should round trip for composite subsets, and they don't currently.

It sounds like the easiest solution to me to just not support CompoundRegions at all to represent Composite Subsets, and use the current expected input format @javerbukh wrote for Subset Tools.import_regions which is corresponding list of subsets and recognized combination modes. I think this is neater honestly - you can only combine two subsets with CompoundRegion so they become nested when combining many which is more confusing than a list. You won't be able to load any arbitrary CompoundRegion that may have been created outside jdaviz, but you can't anyway unless we figure out how to map the 'operator' to a glue combination mode.

for spatial subsets Add specviz pondering, update imviz

pllim · 2024-12-10T17:00:48Z

Re: #3320 (review)

@cshanahan1 , the complication with your suggestion is the serialization. We would have to roll our own data format if user decides to want to save these spatial composite subsets, send the file to collaborator, and then have the collaborator load the file and get back exactly the same thing. Furthermore, such a specialized file format would be useless outside of Jdaviz (unlike the formats supported natively by regions).

pllim · 2024-12-10T17:02:21Z

Re: #3320 (comment)

@javerbukh , if I understood your comment correctly, you are accepting my recommendations in both notebooks without any request for changes. Is that correct?

pllim · 2024-12-10T17:17:01Z

Re: #3320 (comment)

@camipacifici , if I understood your comments correctly, these would be the new recommendations (instead of the ones I put at the end of specviz_composite_subset_api.ipynb. Is that correct?

Acknowledge that original inputs might be lost upon saving it back out because we simplify the composite spectral regions.
Acknowledge that one file can only one subset per row; i.e., "new" is the new default now in the API. However, note that this does break roundtripping because there is no way to store composite regions all in the same row. This limitation needs to be documented clearly with instructions on how user can manually recombine subsets in GUI to (re)make a composite subset.
Because of (2) above, only allow combination_mode="new" when a filename is given. Since default is actually None (which glue translated to "replace", maybe have to recode None to mean "new" when input is filename. Raise exception for any other value.

maybe the csv file could carry the information about the combination?

This gives me the same concern as #3320 (comment) ; i.e., we will end up with a custom file format that is useless outside of Jdaviz.

Is this what could be fixed by building the equivalent of compound_region in specutils?

Yes, theoretically. But given specutils covers more use cases than we do and has a different set of developers, this work might be non-trivial and could take a long time to converge.

camipacifici · 2024-12-11T20:34:54Z

I am ok with this new set of recommendations with two additions:

we will need methods to recombine subsets via API and UI after importing. I do not see tickets for this, but maybe I missed them?
have a ticket in specutils to see if there is something that can be done about compound regions.

pllim · 2024-12-12T16:20:47Z

Note to self: Cross check with a list of tickets from @cshanahan1 (🐱 🐱) and then open tickets.

pllim · 2024-12-13T20:10:41Z

@camipacifici

we will need methods to recombine subsets via API and UI after importing. I do not see tickets for this, but maybe I missed them?

Is this the ticket? https://jira.stsci.edu/browse/JDAT-4533 (from #2871). Update: Created https://jira.stsci.edu/browse/JDAT-5040

have a ticket in specutils to see if there is something that can be done about compound regions

I opened https://jira.stsci.edu/browse/JDAT-5033

Compound spectral region astropy/specutils#1205

pllim · 2024-12-13T20:18:52Z

notebooks/concepts/imviz_composite_subset_api.ipynb

+    "\n",
+    "### Recommendations\n",
+    "\n",
+    "1. Acknowledge that this feature will never be used for aperture photometry.\n",


https://jira.stsci.edu/browse/JDAT-5034

pllim · 2024-12-13T20:35:19Z

notebooks/concepts/imviz_composite_subset_api.ipynb

+    "### Recommendations\n",
+    "\n",
+    "1. Acknowledge that this feature will never be used for aperture photometry.\n",
+    "2. Create follow-up ticket to add support for [compound region](https://astropy-regions.readthedocs.io/en/latest/compound.html).\n",


https://jira.stsci.edu/browse/JDAT-5035

pllim · 2024-12-13T20:38:00Z

notebooks/concepts/imviz_composite_subset_api.ipynb

+    "\n",
+    "1. Acknowledge that this feature will never be used for aperture photometry.\n",
+    "2. Create follow-up ticket to add support for [compound region](https://astropy-regions.readthedocs.io/en/latest/compound.html).\n",
+    "3. Create follow-up ticket to then investigate REPLACE and REMOVE, after (1) is implemented."


https://jira.stsci.edu/browse/JDAT-5036

pllim · 2024-12-13T20:42:12Z

notebooks/concepts/specviz_composite_subset_api.ipynb

+    "1. Acknowledge that original inputs might be lost upon saving it back out\n",
+    "   because we simplify the composite spectral regions.\n",


https://jira.stsci.edu/browse/JDAT-5037

pllim · 2024-12-13T20:55:36Z

notebooks/concepts/specviz_composite_subset_api.ipynb

+    "2. Acknowledge that one file can only store one subset.\n",
+    "   (Needs extra pondering if you also want to support `\"new\"` to create one subset\n",
+    "   per file entry.)\n",
+    "4. Only allow `combination_mode=\"or\"` when a filename is given.\n",
+    "   Since default is actually `None` (which `glue` translated to `\"replace\"`,\n",
+    "   maybe have to recode `None` to mean `\"or\"` when input is filename.\n",
+    "   Raise exception for any other value."


https://jira.stsci.edu/browse/JDAT-5038 (modified with Cami's comments)

pllim · 2024-12-13T21:14:27Z

Tickets are opened. Closing without merging.

pllim added the no-changelog-entry-needed changelog bot directive label Nov 25, 2024

pllim added this to the 4.1 milestone Nov 25, 2024

github-actions bot added the documentation Explanation of code and concepts label Nov 25, 2024

pllim force-pushed the much-subset-so-composite branch from 7e44815 to 1d9b57c Compare November 26, 2024 22:52

pllim marked this pull request as ready for review November 26, 2024 22:53

pllim requested review from rosteen, javerbukh, kecnry, bmorris3, haticekaratay, cshanahan1 and gibsongreen as code owners November 26, 2024 22:53

cshanahan1 mentioned this pull request Dec 9, 2024

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

Merged

cshanahan1 reviewed Dec 9, 2024

View reviewed changes

Concept notebook for composite subset

89cffa5

for spatial subsets Add specviz pondering, update imviz

pllim force-pushed the much-subset-so-composite branch from 1d9b57c to 89cffa5 Compare December 10, 2024 15:22

pllim commented Dec 13, 2024

View reviewed changes

pllim closed this Dec 13, 2024

pllim deleted the much-subset-so-composite branch December 13, 2024 21:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Concept notebooks for composite subset to scope out work #3320

Concept notebooks for composite subset to scope out work #3320

pllim commented Nov 25, 2024 •

edited

Loading

codecov bot commented Nov 26, 2024 •

edited

Loading

javerbukh commented Dec 6, 2024

pllim commented Dec 9, 2024

camipacifici commented Dec 9, 2024

camipacifici commented Dec 9, 2024

cshanahan1 left a comment

pllim commented Dec 10, 2024

pllim commented Dec 10, 2024

pllim commented Dec 10, 2024 •

edited

Loading

camipacifici commented Dec 11, 2024

pllim commented Dec 12, 2024 •

edited

Loading

pllim commented Dec 13, 2024 •

edited

Loading

pllim Dec 13, 2024

pllim Dec 13, 2024

pllim Dec 13, 2024

pllim Dec 13, 2024

pllim Dec 13, 2024

pllim commented Dec 13, 2024

		"1. Acknowledge that original inputs might be lost upon saving it back out\n",
		" because we simplify the composite spectral regions.\n",

Concept notebooks for composite subset to scope out work #3320

Concept notebooks for composite subset to scope out work #3320

Conversation

pllim commented Nov 25, 2024 • edited Loading

Description

TODO for reviewers

Change log entry

Checklist for package maintainer(s)

codecov bot commented Nov 26, 2024 • edited Loading

Codecov Report

javerbukh commented Dec 6, 2024

pllim commented Dec 9, 2024

camipacifici commented Dec 9, 2024

camipacifici commented Dec 9, 2024

cshanahan1 left a comment

Choose a reason for hiding this comment

pllim commented Dec 10, 2024

pllim commented Dec 10, 2024

pllim commented Dec 10, 2024 • edited Loading

camipacifici commented Dec 11, 2024

pllim commented Dec 12, 2024 • edited Loading

pllim commented Dec 13, 2024 • edited Loading

pllim Dec 13, 2024

Choose a reason for hiding this comment

pllim Dec 13, 2024

Choose a reason for hiding this comment

pllim Dec 13, 2024

Choose a reason for hiding this comment

pllim Dec 13, 2024

Choose a reason for hiding this comment

pllim Dec 13, 2024

Choose a reason for hiding this comment

pllim commented Dec 13, 2024

pllim commented Nov 25, 2024 •

edited

Loading

codecov bot commented Nov 26, 2024 •

edited

Loading

pllim commented Dec 10, 2024 •

edited

Loading

pllim commented Dec 12, 2024 •

edited

Loading

pllim commented Dec 13, 2024 •

edited

Loading