spacetelescope / stdatamodels

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

schema_editor: sort listdir output, copy schema before merging #227

Closed braingram closed 9 months ago

braingram commented 11 months ago

Testing locally this will break at least 1 jwst regression test which relies on the bugs fixed in this PR. Running test_full_run[core.schema.yaml] with the changes in this PR (on my mac) the test fails with:

E       @@ -243,7 +243,7 @@
E                    type: string
E                    fits_keyword: OBSFOLDR
E                  time_end:
E       -            title: UTC time at end of exposure
E       +            title: UTC time at end of guiding data
E                    type: string
E                    fits_keyword: TIME-END
E              visit:
E       @@ -1221,9 +1221,9 @@
E                    type: number
E                    fits_keyword: DITH_DEC
E                  subpixel_number:
E       -            title: Subpixel pattern number
E       -            type: integer
E       -            default: 1
E       +            title: Subpixel sampling pattern number
E       +            type: integer
E       +            default: 0
E                    fits_keyword: SUBPXNUM
E              ephemeris:
E                title: JWST ephemeris information

More details about the subpixel_number error are in #225 (and #226). In brief, during the merging of keyword files, schema_editor relies on the arbitrary ordering of files from os.listdir and modifies schemas (in memory). The time_end error is similar (as the title in guidestar.observation.schema.json does not match science.observation.schema.json) and without the changes in this PR the call to create_dict during creation of the schema modified the guidestar schema in memory overwriting it's title to UTC time at end of exposure.

Fixes: #225 Fixes: #226

Resolves JP-nnnn Resolves RCAL-nnnn

Closes #

This PR addresses ...

Checklist

codecov[bot] commented 11 months ago

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (fc890e4) 64.70% compared to head (5b21bb1) 64.70%.

Files Patch % Lines
src/stdatamodels/jwst/datamodels/schema_editor.py 25.00% 3 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #227 +/- ## ======================================= Coverage 64.70% 64.70% ======================================= Files 102 102 Lines 5666 5667 +1 ======================================= + Hits 3666 3667 +1 Misses 2000 2000 ```

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

braingram commented 11 months ago

Running only the test_full_run test_schema_editor regression tests on jenkins (linux) yields the same error as above: https://plwishmaster.stsci.edu:8081/blue/organizations/jenkins/RT%2FJWST-Developers-Pull-Requests/detail/JWST-Developers-Pull-Requests/995/pipeline/199

E       @@ -243,7 +243,7 @@
E                    type: string
E                    fits_keyword: OBSFOLDR
E                  time_end:
E       -            title: UTC time at end of exposure
E       +            title: UTC time at end of guiding data
E                    type: string
E                    fits_keyword: TIME-END
E              visit:
E       @@ -1221,9 +1221,9 @@
E                    type: number
E                    fits_keyword: DITH_DEC
E                  subpixel_number:
E       -            title: Subpixel pattern number
E       -            type: integer
E       -            default: 1
E       +            title: Subpixel sampling pattern number
E       +            type: integer
E       +            default: 0
E                    fits_keyword: SUBPXNUM
E              ephemeris:
E                title: JWST ephemeris information

Running the same tests using the jenkins devdeps job (which also run the tests on a mac) show that now the test fails in the same way on both mac and linux. https://plwishmaster.stsci.edu:8081/blue/organizations/jenkins/RT%2FJWST-devdeps/detail/JWST-devdeps/689/tests

A full regression test run (on linux) is here: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1002/

It shows only the above expected error.