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.

Sign up for GitHub

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

Jump to bottom

DM-28236: get('calexp_filterLabel') does not return a full label for pre-FilterLabel data #341

Merged
merged 4 commits into from Jan 13, 2021
Merged

DM-28236: get('calexp_filterLabel') does not return a full label for pre-FilterLabel data #341

merged 4 commits into from Jan 13, 2021

Conversation

kfindeisen
Copy link
Member

This PR fixes an issue where getting a FilterLabel object from the Gen 2 Butler gave different results depending on the retrieval method. Some reworking of both the Gen 2 code and its unit tests was needed.

parejkoj
parejkoj reviewed Jan 12, 2021
View reviewed changes
Copy link
Collaborator

@parejkoj parejkoj left a comment

Choose a reason for hiding this comment

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

A couple of comments: the logic for this is complicated, but I don't think we have any choice in that matter.

I don't understand why the change from r to i helps? The commit message didn't clear it up for me.

python/lsst/obs/base/cameraMapper.py Show resolved Hide resolved
python/lsst/obs/base/cameraMapper.py Show resolved Hide resolved
tests/test_cameraMapper.py Show resolved Hide resolved
tests/test_cameraMapper.py Show resolved Hide resolved
@kfindeisen
Copy link
Member Author

I don't understand why the change from r to i helps? The commit message didn't clear it up for me.

The test data is a version 0 (or non-afw?) file with a FILTER='r' header keyword. With the old set of test filters, it was impossible to patch in a physical filter, because band='r' was ambiguous (especially since the test repo is configured to not use filters in data IDs). Since patching didn't do anything, it was impossible to test whether or not it happened. Now that there's only one r-band filter, I can assume that there should be a physical filter.

@kfindeisen
Factor filter selection from CameraMapper._setFilter.
5320fc3 
This factoring makes it possible to use the same logic in contexts
other than Exposure.
@kfindeisen
Add real exposure to MinMapper2 tests.
6e13d9a 
This dataset allows for testing of exposure components.
@kfindeisen
Switch CameraMapper standardizeFilter tests to i-band.
bb728a8 
Switching to i-band makes the "r" filter used in test data unambiguous,
which simplifies tests that use those files.
@kfindeisen
Refine filters loaded from exposures using '_filterLabel'.
12c8080 
The same post-processing is now applied to filters loaded directly and
as part of an Exposure, providing a consistent result no matter how
the filter is read.
@kfindeisen kfindeisen merged commit 6948e2c into master Jan 13, 2021
@kfindeisen kfindeisen deleted the tickets/DM-28236 branch January 13, 2021 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parejkoj parejkoj parejkoj left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@kfindeisen @parejkoj