spacetelescope / stdatamodels

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

JP-3558: update integration_number dtype in group schema #283

Closed emolter closed 6 months ago

emolter commented 6 months ago

Resolves JP-3558

Closes #8323

This PR prevents values from wrapping around the limit of the int16 range in observations with a large number of integrations.

Checklist

codecov[bot] commented 6 months ago

Codecov Report

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

Project coverage is 64.92%. Comparing base (4d7c3a6) to head (577b77b). Report is 12 commits behind head on main.

:exclamation: Current head 577b77b differs from pull request most recent head bac0cf6. Consider uploading reports for the commit bac0cf6 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #283 +/- ## ========================================== + Coverage 64.84% 64.92% +0.08% ========================================== Files 103 104 +1 Lines 5694 5710 +16 ========================================== + Hits 3692 3707 +15 - Misses 2002 2003 +1 ```

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

braingram commented 6 months ago

The change looks good to me.

Is there a regression test run?

Also, is this used in jwst? If so would you point me to the usage? I failed in my (albeit brief) search.

emolter commented 6 months ago

@braingram This was triggered by this ticket, related to issues with the time stamps in tso3 white light curves. The datamodel keyword int_times is used several times in the files tso_photometry.py and white_light.py.

Regression test run started here.

braingram commented 6 months ago

Thanks for the reply. Perhaps I'm missing something but doesn't the ticket refer to a different schema: https://github.com/spacetelescope/stdatamodels/blob/41d4a43c1bec37d097a25866c1a76d93460ee306/src/stdatamodels/jwst/datamodels/schemas/int_times.schema.yaml#L9-L12

emolter commented 6 months ago

It looks like you're correct, I had not realized that integration_times shows up in two different places. The one in the int_times schema is already 32-bit, and the JSDP ticket talks about how the wrap was happening unexpectedly despite the already-existing 32-bit definition.

I jumped into this in the middle; the JP ticket does explicitly state to change integration_times in the group schema. So perhaps this ticket was just to ensure consistency between the numbers in the two places? They are meant to contain the same information, right?

@stscirij can you comment on this?

stscirij commented 6 months ago

Yes, as far as I know they should have the same information. The tso code uses the information from the INT_TIMES extension. However the same information is in the GROUP extension, and the datatype in that table is i2 (16 bit). So that also needs to be updated so that it can hold the 32 bit value that is now being used to fill the column in the INT_TIMES extension. I'll submit a new JSDP ticket to cover that work.

emolter commented 6 months ago

There are a bunch of regtest failures, but it looks like the steps ran fine, and all of them just require OKify on the truth files. All 51 of them read:

     Data contains differences:
       Extra column integration_number of format J in a
       Extra column integration_number of format I in b

At what stage should that OKify be run? I guess as soon as this gets merged?

stscirij commented 6 months ago

Looks like https://jira.stsci.edu/browse/JWSTKD-559 will already handle the table format specification for the INT_TIMES and GROUP extensions, so no new ticket required

hbushouse commented 6 months ago

I agree that regtest results look fine (just the expected differences in the data type for the column in the GROUP table extension) and they also prove that the code change is backwards compatible with old FITS files that use the old column data type.