-
Notifications
You must be signed in to change notification settings - Fork 6
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-40426: Add new LATISS filters collimator and cyl_lens, and new gratings holo4_001 and pinhole mask options. #467
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a couple of comments before I do the review.
python/lsst/obs/lsst/filters.py
Outdated
@@ -291,10 +291,41 @@ def addFilter(filter_dict, band, physical_filter): | |||
band="z"), | |||
FilterDefinition(physical_filter="SDSSy_65mm", | |||
band="y"), | |||
FilterDefinition(physical_filter="collimator", | |||
band="collimator"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We try not to name bands after filters if there is another option. We have a current list of: diffuser, u, g, r, i, z, y, unknown, notch, white. Do collimator and cyl_lens have more general band names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They do not have more general band names, but among that list I think either "white" or "unknown" would be the most appropriate as these two elements do not have any intentional attenuation by wavelength.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use white
then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation to use white
adopted and pushed.
"pinhole_2_4_1000", | ||
"pinhole_2_4_0500", | ||
"pinhole_2_4_0200", | ||
"pinhole_2_4_0100", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the gratings turning up in files with these names or is there a requirement to derive the names from multiple FITS headers in the metadata translator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The gratings should be turning up in files with one of these names at a time.
To expand a bit, the plan is to install the pinhole mask into the grating stage instrument with one of these names depending on the pinhole configuration we setup outside of the instrument, before the mask is installed. We chose this list of names to cover all possible configurations as we expect it to change over time. However, that change is always done as part of a grating swap.
6ef8f62
to
6799c47
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Jenkins run -> https://rubin-ci.slac.stanford.edu/job/stack-os-matrix/36/
Note that the PR is organized in 3 separate commits, one for the new filters, one for the new grating, and the third is for the options of the pinhole masks. The pinhole masks (there are two) will only be installed in the grating wheel, but they have configurable options as far as the aperture size and aperture locations. The names were chosen to express all available configurations as it is unknown ahead of time which aperture size/location configuration will be used.