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-40426: Add new LATISS filters collimator and cyl_lens, and new gratings holo4_001 and pinhole mask options. #467

Merged
merged 3 commits into from
Aug 18, 2023

Conversation

edennihy
Copy link
Contributor

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.

@edennihy edennihy requested a review from timj August 18, 2023 19:46
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.

I have a couple of comments before I do the review.

@@ -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"),
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe use white then.

Copy link
Contributor Author

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",
Copy link
Member

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?

Copy link
Contributor Author

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.

@edennihy edennihy changed the title Tickets/dm 40426 DM-40426: Add new LATISS filters collimator and cyl_lens, and new gratings holo4_001 and pinhole mask options. Aug 18, 2023
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.

Looks good.

@edennihy edennihy merged commit 0219084 into main Aug 18, 2023
3 checks passed
@edennihy edennihy deleted the tickets/DM-40426 branch August 18, 2023 22:55
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.

2 participants