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-23090: Add LATISS commissioning filters #158

Merged
merged 6 commits into from Jan 16, 2020
Merged

DM-23090: Add LATISS commissioning filters #158

merged 6 commits into from Jan 16, 2020

Conversation

timj
Copy link
Member

@timj timj commented Jan 16, 2020

First attempt at adding the 4 commissioning filters. I've completely replaced the previous LSST filters that were in the LATISS definition.

Also some minor additional code cleanups since I was looking.

The args should be returned without running the command,
and the command should not call sys.exit itself in main()
Implements RFC-650. Only one line needed to be changed.
Copy link

@patrickingraham patrickingraham left a comment

Choose a reason for hiding this comment

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

Have a few questions...

lambdaEff=364.59, lambdaMin=324.0, lambdaMax=395.0),
FilterDefinition(physical_filter="g",
FilterDefinition(physical_filter="blank_bk7_wg05",
lambdaEff=0.0),

Choose a reason for hiding this comment

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

if lambdaEff=0.0 are lambdaMin and lambdaMax not needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

That seems to be the implication from looking elsewhere.

doc/lsst.obs.lsst/index.rst Show resolved Hide resolved
@@ -144,8 +144,8 @@ def __init__(self, inputPolicy=None, **kwargs):
LsstCamMapper._nbit_patch = 5
LsstCamMapper._nbit_filter = 6

LsstCamMapper._nbit_id = 64 - (LsstCamMapper._nbit_tract + 2*LsstCamMapper._nbit_patch +
LsstCamMapper._nbit_filter)
LsstCamMapper._nbit_id = 64 - (LsstCamMapper._nbit_tract + 2*LsstCamMapper._nbit_patch

Choose a reason for hiding this comment

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

This is rather unclear to me what this is doing...
There is a limit on how many filters you can declare to afw?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not obvious but the important thing for this pull request is that I moved the plus from the end of one line to the start of the next because of an RFC that got adopted yesterday.

Choose a reason for hiding this comment

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

OK, please link the RFC to the ticket prior to merging.

Copy link

@patrickingraham patrickingraham left a comment

Choose a reason for hiding this comment

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

Approved so long as RFC is linked.

@timj timj merged commit 71e49ed into master Jan 16, 2020
@timj timj deleted the tickets/DM-23090 branch February 5, 2020 23:17
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.

None yet

2 participants