DM-38882: Update LSSTCam and TS8 physical filter names #451
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
jchiang87
commented
May 19, 2023
jchiang87
commented
- The LSSTCam physical filter names are provided.
- Filter combinations for use with the CCOB are also added for LSSTCam and TS8.
Have both LSSTCam and TS8 use the base class .to_physical_filter implementation
… headers with new filter keyword handling
jchiang87
force-pushed
the
tickets/DM-38882
branch
from
0eab561
to
e4c1369
Compare
timj
approved these changes
May 19, 2023
| @@ -662,6 +662,14 @@ def to_physical_filter(self): | |||
| if not joined: | |||
| joined = "unknown" | |||
|
|
|||
| # Remove blank and "empty" fields. | |||
| joined = FILTER_DELIMITER.join(_ for _ in joined.split(FILTER_DELIMITER) | |||
There was a problem hiding this comment.
Joining and then splitting seems and joining again seems wrong to me but I can see that you don't want to duplicate the keyword retrieval logic in _join_keyword_values. Looking at _join_keyword_values it might have a bug in it in that it allow empty string to be joined which seems unlikely to be what people want. I could add an extra parameter that allows you to specify values that should be treated as missing and then we wouldn't need this splitting here (although then you might not be able to tell the difference between "unknown" and "empty").
For now I guess we can leave it as it is.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.