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-39346: Fixes for LSSTCam and TS8 filter definitions #454

Merged
merged 5 commits into from May 24, 2023

Conversation

jchiang87
Copy link
Contributor

Also added test code to check header examples against filter definitions for each instrument.

include GENERIC_FILTER_DEFINITIONS in __all__
ensure 'empty' isn't prepended for CCOB filter combinations when FILTER keyword is null
include LsstCamFiltersGeneric in imSim filter defs
@jchiang87 jchiang87 requested a review from timj May 24, 2023 14:28
Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

This looks okay. I'm not entirely sure how the stripping of the ~empty is now handled in TS8 now that you moved the stripping to LSSTCam.

@@ -46,7 +46,7 @@ DARKTIME: 15.165
TSEQNUM: 18
FPVERS: '1.1.8-SNAPSHOT'
IHVERS: '1.0.36'
FILTER: ''
FILTER: null
Copy link
Member

Choose a reason for hiding this comment

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

How were these yaml files created? astrometadata dump does correctly put null if the keyword value is undefined. Is this specific file using an empty string? In which case it needs to stay an empty string and the physical filter code needs to use is_key_ok() which checks for truth and so handles empty string and undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just used astropy.io.fits and enclosed keyword values that are returned as strings in single quotes. I wasn't aware of astrometadata dump. When I was debugging, is_key_ok() was definitely returning false for that FILTER keyword when run on current TS8 data.

Copy link
Member

Choose a reason for hiding this comment

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

Okay. astrometadata dump exists precisely to make my life easier when generating these YAML files.

Isn't False what you wanted for is_key_ok() for empty string and undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this context, it's what I want, since TS8 is writing those keywords with null values. I would prefer not have to deal with an empty string, however, where currently is_key_ok() seems to return True.

Returns "unknown" if no filter is declared.
"""
joined = super().to_physical_filter()
while joined.endswith("~empty"):
Copy link
Member

Choose a reason for hiding this comment

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

I think this will work:

return re.sub("(~empty)+$", "", joined)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll leave this for now. I suspect this code may need to be revisited once LSSTCam data taking starts up again, so I'll fix it then.

Copy link
Member

Choose a reason for hiding this comment

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

People don't like regexps for some reason... 😉

@jchiang87
Copy link
Contributor Author

jchiang87 commented May 24, 2023

TS8 doesn't currently produce filter keyword combinations that result in physical_filter values that have the trailing ~empty. The practice of explicitly writing filter keywords with the empty string was an LSSTCam thing, and I understand will not be continued with the new data taking.

@jchiang87 jchiang87 merged commit 21ea1d0 into main May 24, 2023
2 checks passed
@jchiang87 jchiang87 deleted the tickets/DM-39346 branch May 24, 2023 17:56
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

2 participants