-
Notifications
You must be signed in to change notification settings - Fork 13
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 more spectral-cube class translators #54
Conversation
Codecov ReportBase: 97.45% // Head: 97.15% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #54 +/- ##
==========================================
- Coverage 97.45% 97.15% -0.30%
==========================================
Files 17 17
Lines 1219 1232 +13
==========================================
+ Hits 1188 1197 +9
- Misses 31 35 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
@keflavich does this need more work or could it be rebased to re-run the tests? |
bd41794
to
9299277
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The missing import and the flake8 errors seem to be the only remaining failures.
@keflavich with the codestyle fixed and all tests passing, this could in principle be merged, if a changelog entry is added. Tests for the new formats would be nice as well, of course, but the main point I am wondering about is whether the translator provides enough functionality to be useful at this point. |
I don't understand enough about I'd be in favor of merging this now and opening new issues for additional functionality. |
And I don't know enough about beams... but basically every actual data object in glue is a |
Co-authored-by: Derek Homeier <dhomeie@gwdg.de>
1d44888
to
79da5dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have information about the beam size in each pixel it could be stored as a component, otherwise it will need to be stored in .meta
. I think this is fine as-is - I would have liked to see a test included here for the new functionality but I also won't object if this is merged and tests are added later.
The
SpectralCube
handlers didn't work for any but the basic single-beam SpectralCube objects. This adds support for the other types.I haven't tested this extensively yet, but I think the ideas are sound - as long as the metadata is appropriately preserved.
Problem:
beams
needs to be subsetted along with the data. How can we handle that?