DM-41926: Add "NONE" physical_filter for LSSTCam data #491
Conversation
| # and all of the various ways that may be expressed in the FITS | ||
| # header. | ||
| NoFilterCollection = FilterDefinitionCollection( | ||
| FilterDefinition(physical_filter="empty", band="white", |
There was a problem hiding this comment.
What does empty mean if it doesn't mean NONE? If empty and NONE are the same thing I don't know why we need two definitions. If they are two different states then can we add some comments about those different states here?
all of the various ways that may be expressed in the FITS header
is not a reason to add all those various ways into the phyiscal filter definition. The purpose of the metadata translation stage before this is to handle all the various ways consistently so that we don't get questions about the difference between empty and none.
There was a problem hiding this comment.
I am perfectly happy to not do this change at all. Yousuke and Tony seemed to have a preference to have both of these methods available. If we think that's unreasonable, we should note that on the ticket and just put in the fix-up you suggested to map "NONE" to "empty".
There was a problem hiding this comment.
If NONE and empty mean the same thing but people prefer to write NONE in the header, then I really think that we should map NONE to empty in the metadata translator instead. If there are two distinct states where NONE and empty convey different meanings then we should have both.
There was a problem hiding this comment.
If NONE and empty mean the same thing but people prefer to write NONE in the header, then I really think that we should map NONE to empty in the metadata translator instead.
Sounds good to me.
@tony-johnson Are these distinct states?
There was a problem hiding this comment.
They are different. The "NONE" state corresponds to the state where we don't have anything in the optical path. The empty state corresponds to the state where we have the autochanger in the optical path. The autochanger is the thing that holds filters in the optical path. They could be different in terms of how does the edge get blocked.
There was a problem hiding this comment.
Based on Tony's comment here, "NONE" corresponds to the partially occluded case. I'll assume that "empty" refers to the un-occluded case.
There was a problem hiding this comment.
@timj I've updated the comments so that they describe the different states. Please have another look.
…ing no filter in the FITS header; add test data
jchiang87
force-pushed
the
tickets/DM-41926
branch
from
e1649e6
to
13ab1e3
Compare
python/lsst/obs/lsst/filters.py
Outdated
| # For this case, all filters are returned to the carousel and the | ||
| # auto-changer partially occludes the focal plane. See Tony | ||
| # Johnson's comment at https://jira.lsstcorp.org/browse/DM-41675. | ||
| FilterDefinition(physical_filter="NONE", band="white", |
There was a problem hiding this comment.
I would rather this was none to keep it consistent with empty case. The translator can easily map NONE -> none.
There was a problem hiding this comment.
I've made the change, but I'm open to alternative suggestions for mapping NONE->none.
There was a problem hiding this comment.
I don't have any opinion whether NONE or none as long as it's not blocking the current data ingestion.
There was a problem hiding this comment.
Looks okay. I now get:
$ butler query-dimension-records xxx physical_filter | grep none
LSSTCam none white
LSSTCam none~HIGH white
LSSTCam none~LOW white
LSSTCam none~blue g
LSSTCam none~nm750 i
LSSTCam none~nm850 z
LSSTCam none~nm960 y
LSSTCam none~red r
LSSTCam none~uv u
| @@ -673,6 +673,9 @@ def to_physical_filter(self): | |||
| if not joined: | |||
| joined = "unknown" | |||
|
|
|||
| # Replace instances of "NONE" with "none". | |||
| joined = joined.replace("NONE", "none") | |||
There was a problem hiding this comment.
This is fine so long as we don't have any bigger strings with NONE in.
No description provided.