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

Generalize PaddedSpectrumWCS for higher dimensionality #68

Merged
merged 14 commits into from
Mar 22, 2022

Conversation

rosteen
Copy link
Contributor

@rosteen rosteen commented Mar 16, 2022

Description

This is an alternative to #61 that rudely ignores @astrofrog's comment explicitly saying not to do this 😆 . The upshot is that it was far more clear to me how to do this than how to finish out the other PR, and we do need to fix #60 as soon as possible.

Note that I've also taken the opportunity to stop reordering the axes of the data at all here, leaving it to the user to handle Spectrum1D order with the spectral axis last (as opposed to SpectralCube, which has that axis first). That seems far better than swapping axes for 3D data but not 2D (or 4D?) data for reasons that were really downstream and shouldn't have been on glue-astronomy to deal with anyway. This means that, if this does end up getting merged, it will require some fixes to be ready in jdaviz (I'm working on these now).

@codecov
Copy link

codecov bot commented Mar 16, 2022

Codecov Report

Merging #68 (65c0708) into main (d87a94d) will increase coverage by 0.34%.
The diff coverage is 97.46%.

@@            Coverage Diff             @@
##             main      #68      +/-   ##
==========================================
+ Coverage   96.61%   96.95%   +0.34%     
==========================================
  Files          15       15              
  Lines        1151     1182      +31     
==========================================
+ Hits         1112     1146      +34     
+ Misses         39       36       -3     
Impacted Files Coverage Δ
glue_astronomy/translators/spectrum1d.py 94.83% <93.75%> (+2.27%) ⬆️
...lue_astronomy/translators/tests/test_spectrum1d.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d87a94d...65c0708. Read the comment docs.

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

This looks great, thanks! Small comment below, and also wanted to check if you could add some simple tests for the methods/properties that codecov is complaining about in the diff, just as we might as well take this opportunity to make sure they work fine (as glue relies on some of them).

Also thank you thank you thank you for no longer re-ordering axes 🙇 😆

Once this is merged in, I guess we should fix jdaviz to be able to use this version, then I can tag a release of glue-astronomy. We should maybe talk at the SME tag-up next week about how to coordinate releases of glue-astronomy and jdaviz to make sure we don't impact users.

@@ -42,24 +42,25 @@ class PaddedSpectrumWCS(BaseWCSWrapper, HighLevelWCSMixin):
# generalize this we can just remove this class and use CompoundLowLevelWCS
Copy link
Member

Choose a reason for hiding this comment

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

Update comment above

@rosteen
Copy link
Contributor Author

rosteen commented Mar 17, 2022

Thanks for the comments, I'll work on adding some more test coverage after I figure out why one of the existing tests is failing. I have a draft PR open in Jdaviz where I'm already starting to work through the updates needed to accommodate these changes - we have a half year build due for Jdaviz at the end of next week, so I'm hoping to finish this up tomorrow or early next week to release by then. Agreed that tagup next week will be a good time to discuss.

@rosteen rosteen force-pushed the spec3d-generalize-padded branch 2 times, most recently from d71bb15 to a82f3a3 Compare March 21, 2022 15:43
@rosteen rosteen changed the title Spec3d generalize padded Generalize PaddedSpectrumWCS for higher dimensionality Mar 21, 2022
@rosteen rosteen marked this pull request as ready for review March 21, 2022 15:46
@rosteen
Copy link
Contributor Author

rosteen commented Mar 21, 2022

I fixed a few remaining issues with the world<->pixel conversion and added test coverage for the 3D case, I think this is ready for final review now. I still have a decent bit of work on the related Jdaviz PR.

Edit: I'll add checks on the WCS properties in the tests shortly, forgot about those.

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

This looks good to me, can you add a changelog entry since this will be an API change? You can add a v0.4.0 version to the changelog.

@astrofrog
Copy link
Member

@rosteen - it looks like this needs a rebase because of the recent changelog changes.

@rosteen
Copy link
Contributor Author

rosteen commented Mar 22, 2022

Done, could have sworn I did that before my last push but maybe the timing meant I missed a commit.

@astrofrog astrofrog merged commit db72dbc into glue-viz:main Mar 22, 2022
@astrofrog
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spectrum1D translator cannot handle spectral_axis in 3D cube
3 participants