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

safely convert FITS_rec for non-schema data #268

Merged
merged 3 commits into from
Mar 20, 2024

Conversation

braingram
Copy link
Collaborator

@braingram braingram commented Feb 29, 2024

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:

  /data1/jenkins/workspace/RT/JWST-devdeps/miniconda/envs/tmp_env0/lib/python3.11/site-packages/asdf/yamlutil.py:282: AsdfConversionWarning: A ndarray subclass (<class 'astropy.io.fits.fitsrec.FITS_rec'>) was converted as a ndarray. This behavior will be removed from a future version of ASDF. See https://asdf.readthedocs.io/en/latest/asdf/config.html#convert-unknown-ndarray-subclasses

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:

jwst/regtest/test_fgs_guider.py::test_fgs_guider[exptype_fgs_id_stack-cal]
/data1/jenkins/workspace/RT/JWST-Developers-Pull-Requests/miniconda/envs/tmp_env2/lib/python3.11/site-packages/asdf/yamlutil.py:333: AsdfConversionWarning: tag:stsci.edu:asdf/core/asdf-1.0.0 is not recognized, converting to raw Python data structure

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

#ASDF 1.0.0
#ASDF_STANDARD 1.2.0
%YAML 1.1
%TAG ! tag:stsci.edu:asdf/
--- !core/asdf-1.0.0

The file reports ASDF_STANDARD 1.2.0 yet uses the asdf-1.0.0 tag which should be asdf-1.1.0 in this version of the standard. The same error can be seen by opening the file with stdatamodels. Re-saving the file fixes this issue.

Checklist

  • added entry in CHANGES.rst (either in Bug Fixes or Changes to API)
  • updated relevant tests
  • updated relevant documentation
  • updated relevant milestone(s)
  • added relevant label(s)

Copy link

codecov bot commented Feb 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.95%. Comparing base (4d7c3a6) to head (8cc9b10).
Report is 18 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

@braingram

This comment was marked as resolved.

@braingram braingram force-pushed the non_schema_fitsrec branch 2 times, most recently from 20ac5c2 to abd618b Compare February 29, 2024 15:32
@braingram braingram marked this pull request as ready for review February 29, 2024 18:53
@braingram braingram requested a review from a team as a code owner February 29, 2024 18:53
@braingram
Copy link
Collaborator Author

@emolter This should fix the AsdfConversionWarnings you found in the jwst tests.

@braingram braingram marked this pull request as draft February 29, 2024 20:48
@braingram braingram force-pushed the non_schema_fitsrec branch 2 times, most recently from 9609eeb to 7d0787a Compare February 29, 2024 20:51
@braingram

This comment was marked as resolved.

@braingram
Copy link
Collaborator Author

To provide a bit more context on this PR.

This PR adds a (hopefully) final pass safely converting FITS_rec instances in the ASDF tree before it's saved. If not safely converted FITS_rec instances can do problematic things (like corrupting the input data array or output file, see #280 for one example). This PR is using the "safe" conversion code that exists instdatamodels:

def convert_fitsrec_to_array_in_tree(tree):

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.

Copy link
Collaborator

@tapastro tapastro left a 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!

@braingram braingram merged commit b45e0ac into spacetelescope:main Mar 20, 2024
21 checks passed
@braingram braingram deleted the non_schema_fitsrec branch March 20, 2024 19:59
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.

2 participants