search

spacetelescope / stdatamodels

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

restore column units on cast #242

Closed braingram closed 8 months ago

braingram commented 9 months ago

Resolves JP-3482

Closes [spacetelescope/jwst#8109]

When a DataModel containing a table extension is loaded from a FITS file, stdatamodels currently discards the units defined in the TUNITS headers. This is due to a _cast from a FITS_rec to a ndarray (to protect against the numerous gotchas in FITS_rec): https://github.com/spacetelescope/stdatamodels/blob/b7eada9ee768691bafbb9eb6b38b18c3eb50d85b/src/stdatamodels/properties.py#L86 then a conversion back to a FITS_rec to allow jwst code to continue to interact with a FITS_rec: https://github.com/spacetelescope/stdatamodels/blob/b7eada9ee768691bafbb9eb6b38b18c3eb50d85b/src/stdatamodels/properties.py#L89

This PR is an alternative to #238 and a stop-gap measure until #240 can be resolved.

Checklist

codecov[bot] commented 9 months ago

Codecov Report

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

Comparison is base (bbc9016) 64.78% compared to head (3dd1cfd) 64.82%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #242 +/- ## ========================================== + Coverage 64.78% 64.82% +0.04% ========================================== Files 103 103 Lines 5688 5695 +7 ========================================== + Hits 3685 3692 +7 Misses 2003 2003 ```

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

braingram commented 9 months ago

Regression tests started at: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1084/ the above tests crashed and burned due to many artifactory connection errors I restarted them with the hope that this was transient: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1085/ The re-run shows 30 errors. None of which on the surface appear related to these changes. All contain the following:

E              Extra column R_MIREMI of format D in a
E              Extra column S_MIREMI of format D in a

which points towards truth files that haven't been updated yet for https://github.com/spacetelescope/stdatamodels/pull/200

braingram commented 9 months ago

@jemorrison I can't seem to request you as a reviewer but thanks again for testing out this PR and confirming that it fixed your issue. Please feel free to review/comment on the PR if you have a chance to give it a look.

jemorrison commented 9 months ago

Looks reasonable to me and it works.

hbushouse commented 8 months ago

There have been a couple rounds of regtest truth file updates over the past few days, so it might be worthwhile to run this PR branch again, one last time.

hbushouse commented 8 months ago

OK, now that this has been merged, I'll just kick off a regular RT run, which will use stdatamodels/master.

braingram commented 8 months ago

Thanks! Sorry for the quick draw on the merge.

hbushouse commented 8 months ago

Regtest run https://plwishmaster.stsci.edu:8081/job/RT/job/JWST/2686/ shows only unrelated failures/errors. So this looks good (at least it does no obvious harm).