-
-
Notifications
You must be signed in to change notification settings - Fork 128
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
ASDF mask serialization #1053
ASDF mask serialization #1053
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1053 +/- ##
=======================================
Coverage ? 70.19%
=======================================
Files ? 64
Lines ? 4412
Branches ? 0
=======================================
Hits ? 3097
Misses ? 1315
Partials ? 0 ☔ View full report in Codecov by Sentry. |
Overall these changes look good. One thing to consider is how tag versioning will be handled. If a spectrum that contains a masked array is saved with an older version of specutils, the asdf file (with a 1.0.0 spectrum 1d tag) will not contain the mask (as noted in #1052). With the changes in this PR, the masked spectrum will be saved to the asdf file. However the file will contain the same 1.0.0 spectrum tag. If an older version of the software opens this newer file it will fail to load the mask and no errors or warnings will be raised (but the spectrum will not include the mask). If instead the tag was versioned (bumping it to 1.1.0) than specutils would generate a file with the 1.1.0 tag which would fail to convert for any older versions of specutils (and a AsdfConversionWarning would be issued). If this isn’t a concern (it looks like there has been no release since the converters were added) than a version increase doesn’t seem necessary. |
I am not wise in the ways of ASDF, so I will have to defer to specutils maintainers about version bump or not. |
I don't think it's needed right now, since as said we haven't actually released the converters. |
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.
LGTM, thanks!
Fix #1052
@braingram should also have a look to see if I did this wrong.