Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-39415: Fix TS8 exposure ID calculation #455

Merged
merged 4 commits into from May 25, 2023
Merged

DM-39415: Fix TS8 exposure ID calculation #455

merged 4 commits into from May 25, 2023

Conversation

timj
Copy link
Member

@timj timj commented May 25, 2023

We have already ingested TS8 data into a butler using the old timestamp exposure_id calculation. It is very disruptive to change this because it will require the entire repository be remade from scratch. Furthermore, the fix to DATE-OBS broke all the historical calculations where the exposure_id was being derived from the end of the integration by mistake.

This PR retains LSSTCam exposure_id for > 2023-05-24 and uses the original DATE-OBS value to calculate the exposure ID for older data.

Corrects the changes introduced in #453

Numpy scalar has difficulties converting to YAML and we need
this to be a native float.
@timj timj requested a review from jchiang87 May 25, 2023 21:36
We have already ingested TS8 data into a butler using the old
timestamp exposure_id calculation. It is very disruptive to change
this because it will require the entire repository be remade from
scratch. Furthermore, the fix to DATE-OBS broke all the historical
calculations where the exposure_id was being derived from the end
of the integration by mistake.

This commit retains LSSTCam exposure_id for > 2023-05-24 and
uses the original DATE-OBS value to calculate the exposure ID for
older data.
Copy link
Contributor

@jchiang87 jchiang87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. However, I think there is a separate issue regarding how these changes would affect the behavior of butler register-instrument --update.

@timj
Copy link
Member Author

timj commented May 25, 2023

You mean the visit_max stuff? The --update option doesn't fix that?

@timj
Copy link
Member Author

timj commented May 25, 2023

I think I can fix the max_exposure_id problem pretty quickly.

This restores the TS8 exposure id maximum to a value before
we switched over to using LSSTCam exposure_id. We need this
to take into account the earlier data that use a timestamp
and so a larger exposure ID. We do not change the value since
we do not want to modify existing repositories.
@timj
Copy link
Member Author

timj commented May 25, 2023

Okay. I think I've fixed the max exposure ID.

@timj timj requested a review from jchiang87 May 25, 2023 22:20
Copy link
Contributor

@jchiang87 jchiang87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good (nice use of regexps).

@timj timj merged commit 24ff4a9 into main May 25, 2023
2 checks passed
@timj timj deleted the tickets/DM-39415 branch May 25, 2023 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants