-
Notifications
You must be signed in to change notification settings - Fork 26
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
safely convert FITS_rec for non-schema data #268
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #268 +/- ##
==========================================
+ Coverage 64.84% 64.95% +0.11%
==========================================
Files 103 104 +1
Lines 5694 5712 +18
==========================================
+ Hits 3692 3710 +18
Misses 2002 2002 ☔ View full report in Codecov by Sentry. |
This comment was marked as resolved.
This comment was marked as resolved.
20ac5c2
to
abd618b
Compare
@emolter This should fix the |
9609eeb
to
7d0787a
Compare
This comment was marked as resolved.
This comment was marked as resolved.
7d0787a
to
45daea0
Compare
To provide a bit more context on this PR. This PR adds a (hopefully) final pass safely converting stdatamodels/src/stdatamodels/util.py Line 322 in 8bbc9e8
to deal with a case that was revealed during jwst testing. Specifically, when a fits file is opened that contains a table that is not defined by the datamodel schema. The non-schema-defined table will end up in extra_fits in the ASDF tree. On write, stdatamodels will duplicate this data, writing it once to a FITS extension and also including it in the ASDF extension (serialized to ASDF) (see #271 for more details on this duplication). Without this PR, the table/FITS_rec in the ASDF tree will be passed to asdf in a dangerous state (not safely converted by stdatamodels ). For asdf 3.0 (to maintain backwards compatibility) asdf will see that FITS_rec is a subclass of ndarray and treat it as a normal ndarray (which can lead to data corruption) and issue a warning. With this PR the FITS_rec will be safely converted to an array and the warning and data corruption risks (at least from the asdf side) will be avoided.
|
45daea0
to
8cc9b10
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.
Appreciate the details - looks good!
Convert
FITS_rec
instances read from old files where a hdu was linked in the old schema (but is no longer linked) when rewriting files.Resolves JP-3555
Regression tests run:
https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1250/
As this is aimed to fix warnings like:
which by-default do not produce errors. The log output of the regression test run will need to be manually checked (as I did not start this run with
-W error::asdf.exceptions.AsdfConversionWarning
.No warnings about ndarray subclass conversions are in the above regtest run log.
1 common and unrelated error occurred for
[stable-deps] test_duplicate_names – jwst.associations.tests.test_level3_duplicate
failing to find a log message (likely due to stpipe logging issues).1 related warning can be seen in the regtest run:
This is caused by the regtest input file:
https://bytesalad.stsci.edu/ui/repos/tree/General/jwst-pipeline/dev/fgs/level1b/exptype_fgs_id_stack_uncal.fits?clearFilter=true
containing outdated and invalid asdf
The file reports
ASDF_STANDARD 1.2.0
yet uses theasdf-1.0.0
tag which should beasdf-1.1.0
in this version of the standard. The same error can be seen by opening the file withstdatamodels
. Re-saving the file fixes this issue.Checklist
CHANGES.rst
(either inBug Fixes
orChanges to API
)