spacetelescope / stdatamodels

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

JP-3329: Add GSC_VER to JWST core schema #190

Closed hbushouse closed 1 year ago

hbushouse commented 1 year ago

Resolves JP-3329

Closes spacetelescope/jwst#7791

This PR adds the new "GSC_VER" keyword to the JWST core schema, to keep in synch with the changes made to the keyword dictionary in JWSTKD-534. In addition to adding the definition of GSC_VER, it also makes minor updates to the comment/title field of several other xxx_VER keywords, again in keeping with changes made in the dictionary.

Checklist

codecov[bot] commented 1 year ago

Codecov Report

Patch and project coverage have no change.

Comparison is base (078bd1c) 64.01% compared to head (5ac025d) 64.01%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #190 +/- ## ======================================= Coverage 64.01% 64.01% ======================================= Files 101 101 Lines 5558 5558 ======================================= Hits 3558 3558 Misses 2000 2000 ```

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

braingram commented 1 year ago

Changes look good to me. One question about the title changes. I think these get included in the header keywords as comments. For example:

TELESCOP= 'JWST    '           / Telescope used to acquire the data

Will the header changes cause fitsdiff failures for the regression tests?

hbushouse commented 1 year ago

Will the header changes cause fitsdiff failures for the regression tests?

Yes, definitely. I'm prepared to handle those.

tapastro commented 1 year ago

Changes look good to me. One question about the title changes. I think these get included in the header keywords as comments. For example:

TELESCOP= 'JWST    '           / Telescope used to acquire the data

Will the header changes cause fitsdiff failures for the regression tests?

Probably so - our fitsdiff ignores some keywords (like CAL_VER), but not all of these.

braingram commented 1 year ago

Probably so - our fitsdiff ignores some keywords (like CAL_VER), but not all of these.

Thanks!

Is it a terrible idea to try adding ignore_comments='*' (or something similar) to the fitsdiff_default_kwargs?

hbushouse commented 1 year ago

Probably so - our fitsdiff ignores some keywords (like CAL_VER), but not all of these.

Thanks!

Is it a terrible idea to try adding ignore_comments='*' (or something similar) to the fitsdiff_default_kwargs?

Perhaps not a terrible idea, at least for the regular nightly runs, but I like having the comparison turned on just for the purpose of verifying changes like these.

hbushouse commented 1 year ago

Regression test running at https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/843/

hbushouse commented 1 year ago

Confirmed that all (350!) regtest failures are due to the expected header changes.