glue-viz / glue-astronomy

Plugin to add astronomy-specific functionality to glue
https://glue-astronomy.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
11 stars 12 forks source link

Generalize PaddedSpectrumWCS for higher dimensionality #68

Closed rosteen closed 2 years ago

rosteen commented 2 years ago

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[bot] commented 2 years ago

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%) :arrow_up:
...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.

rosteen commented 2 years ago

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 commented 2 years ago

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.

astrofrog commented 2 years ago

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

rosteen commented 2 years ago

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

astrofrog commented 2 years ago

Thanks!