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.

Sign up for GitHub

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

Jump to bottom

Refactor cubeviz parser and plugins for specutils 2.0 compatibility #2762

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from
Draft

Refactor cubeviz parser and plugins for specutils 2.0 compatibility #2762

wants to merge 22 commits into from

Conversation

rosteen
Copy link
Collaborator

@rosteen rosteen commented Mar 19, 2024 •
edited
Loading

Tests are expected to fail on this, since it requires specutils v2.0-dev and the glue-astronomy compatibility PR to work. But I'd like to get this in front of other people for testing sooner rather than later.

Current known to-do items, I will add to these as new issues they are discovered and update when fixed:

  • Deprecate _parse_jwst_s3d now that specutils and glue-astronomy handle GWCS.
  • Test JWST ESA 3d file to see if _parse_esa_s3d can be offloaded to Spectrum1D.read like _parse_jwst_s3d was.
  • Fix slice plugin getting values from wrong axis.
  • Fix coords info showing 0 or NaN for the value on hover.
  • Verify that coords_info is giving flux for correct pixel.
  • Spectral extraction is failing with an error in the glue-astronomy NDData translator.
  • Aperture photometry fails due to attempting to operate on the wrong axis.

@rosteen rosteen added this to the 3.10 milestone Mar 19, 2024
@github-actions github-actions bot added imviz plugin Label for plugins common to multiple configurations labels Mar 19, 2024
@rosteen
Copy link
Collaborator Author

rosteen commented Mar 19, 2024 •
edited
Loading

I confirmed offline that the ESA parser can be removed, it was specifically to support commissioning.

@rosteen
Copy link
Collaborator Author

rosteen commented Mar 25, 2024

It looks like the pixel coords_info is returning a value for is off by one in the y-direction, not immediately obvious to me why (I don't think GWCS is 1-indexed...).

@rosteen rosteen force-pushed the refactor-cubeviz-parser branch from cc94d7d to 611aa44 Compare April 9, 2024 17:29
rosteen added 21 commits April 18, 2024 10:35
@rosteen
Remove jwst s3d parser now that it can be handled by Spectrum1D
ae654e2
@rosteen
Starting to debug slice
ed4a3a4
@rosteen
Fix coords mouseover when SkyCoord is before SpectralCoord
c5266d1
@rosteen
More debugging
ee0cb79
@rosteen
Fix slice with different spectral_axis_index, add specutils_format kw…
1586e8d 
…arg to cubeviz parser
@rosteen
Remove unused import
fe37147
@rosteen
Remove ESA parser from commissioning [skip ci]
4506524
@rosteen
Generalizing coords_info to handle spectral axis first or last [ci skip]
0eb9cb6
@rosteen
Working on debugging aperture photometry
6565e9d
@rosteen
Need to pull meta from data not comp
1304cca
@rosteen
Fix aperture photometry and wrong coord order in coords_info
b7c27d6
@rosteen
Fix codestyle, delete defunct case in parser
379f98c
@rosteen
Change Spectrum1D to Spectrum
007ae6e
@rosteen
Debugging
eb35397
@rosteen
Change class name in recent code additions
a70e082
@rosteen
Get spectral extraction working again for cylinder case
85c86df
@rosteen
Debugging wavelength dependent case
217a5bd
@rosteen
Make sure spectral extraction result is linked when added to data col…
d832c5c 
…lection
@rosteen
Moved fix to correct place
49e94b3
@rosteen
Fix indents
c65f091
@rosteen
Revert this change from rebase to match main
06819df
@rosteen rosteen force-pushed the refactor-cubeviz-parser branch from d44823c to 06819df Compare April 18, 2024 15:07
@bmorris3 bmorris3 modified the milestones: 3.10, 3.11 May 3, 2024
pllim
pllim reviewed May 14, 2024
View reviewed changes
pyproject.toml
@@ -23,7 +23,7 @@ dependencies = [
"ipywidgets>=8.0.6",
"voila>=0.4,<0.5",
"pyyaml>=5.4.1",
"specutils>=1.9",
"specutils>=2.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as glue-viz/glue-astronomy#100 (review)

Suggested change
"specutils>=2.0",
"specutils>=2.0.dev",

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But then we'll see all the test failures that I haven't updated to fix 😭 😆

@rosteen rosteen mentioned this pull request Jun 20, 2024
Draft
@rosteen rosteen modified the milestones: 3.11, 4.1 Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pllim pllim pllim left review comments

@javerbukh javerbukh Awaiting requested review from javerbukh javerbukh will be requested when the pull request is marked ready for review javerbukh is a code owner

@kecnry kecnry Awaiting requested review from kecnry kecnry will be requested when the pull request is marked ready for review kecnry is a code owner

@bmorris3 bmorris3 Awaiting requested review from bmorris3 bmorris3 will be requested when the pull request is marked ready for review bmorris3 is a code owner

@haticekaratay haticekaratay Awaiting requested review from haticekaratay haticekaratay will be requested when the pull request is marked ready for review haticekaratay is a code owner

@cshanahan1 cshanahan1 Awaiting requested review from cshanahan1 cshanahan1 will be requested when the pull request is marked ready for review cshanahan1 is a code owner

@gibsongreen gibsongreen Awaiting requested review from gibsongreen gibsongreen will be requested when the pull request is marked ready for review gibsongreen is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
cubeviz imviz plugin Label for plugins common to multiple configurations Upstream fix required 💤 enhancement New feature or request
Projects
None yet
Milestone
4.1
Development

Successfully merging this pull request may close these issues.
3 participants
@rosteen @pllim @bmorris3