spacetelescope / stdatamodels

https://stdatamodels.readthedocs.io
Other
5 stars 25 forks source link

JP-2349: Add workaround to load old files with new moving_target_table schema #329

Closed tapastro closed 1 month ago

tapastro commented 2 months ago

Resolves JP-2349

This PR addresses validation issues encountered when opening jwst uncal files generated with DMS Ops builds <11.1 and using stdatamodels>=2.1.0. This most recent release of stdatamodels changed the number of columns in the moving_target_table schema, which leads to validation errors when opening any existing file with an old moving target extension. For any users attempting to load an outdated uncal file, this PR will check for the missing columns and add nan-filled columns if they are missing.

Tasks

news fragment change types... - ``changes/.feature.rst``: new feature - ``changes/.bugfix.rst``: fixes an issue - ``changes/.doc.rst``: documentation change - ``changes/.removal.rst``: deprecation or removal of public API - ``changes/.misc.rst``: infrastructure or miscellaneous change
tapastro commented 2 months ago

I'm running a set of regression tests for completeness: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1751/

But this made it through without a regression test failure, which would mean that we don't have a regression test that loads an uncal file with moving target data. This needs to be fixed in a yet-unwritten PR to jwst with a new test.

EDIT: I remember now - bad management advice from yours truly. @emolter modified the test's input file to include the new columns, which of course fixed the test but ignores the problems introduced for all other files. We can get coverage of this workaround by updating the test_nircam_mtimage to use flight data, as all uncals are still missing these columns (unless handcrafted...)

emolter commented 2 months ago

@emolter modified the test's input file to include the new columns, which of course fixed the test but ignores the problems introduced for all other files.

Guess I learned the hard way that this is bad practice... sorry about that. Should have thought it through more

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 66.55%. Comparing base (1996b18) to head (fd6be8d). Report is 7 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #329 +/- ## ========================================== + Coverage 66.44% 66.55% +0.10% ========================================== Files 102 102 Lines 5439 5456 +17 ========================================== + Hits 3614 3631 +17 Misses 1825 1825 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

tapastro commented 1 month ago

JWST regtests pass with this PR: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1755/

Merging!