-
Notifications
You must be signed in to change notification settings - Fork 7
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-45856: Add new calibration pipelines for IsrTaskLSST to _ingredients #266
Conversation
See #267 for associated PR implementing the LATISS pipelines. |
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 think most of the comments are questions where I don't think I'm following along completely.
doBootstrap: false | ||
doDeferredCharge: false | ||
doBrighterFatter: false | ||
doLinearize: true |
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.
General comment: Are these config options sorted in any fashion, and if not, can they be? Application order would be my preference (with doDiffNonLinearCorrection
first), but even alphabetical would simplify things.
Also, can we use the same quoting ('
vs "
) throughout? I know the inconsistency is likely my fault, but it's throwing a lot of false positives in the diffs I'm doing to check the different pipeline versions.
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.
Argh, I meant to have everything double quoted and I will fix that.
As for ordering, I tried to keep it consistent but then didn't do a further pass. Which should I do ... application order or alphabetical?
connections.outputData: "flatBootstrap" | ||
calibrationType: "flat" | ||
exposureScaling: InputList | ||
scalingLevel: AMP |
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 assume this is AMP
, where cpFlatLSST.yaml
uses DETECTOR
is due to this bootstrap version not having gains to normalize the amplifiers, so it's using the best guess from the normalization code? If so, a comment here might be useful so that it's clear that this difference is intentional.
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 don't understand the normalization conditions so this was at best accidental. This is used to find defects ... so I think AMP is appropriate? Unless you say it should be DETECTOR? Which should it be to find dark defects? None maybe.
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 think AMP
is more correct here, but in thinking about the details of this, I don't think it matters. The defect code measures a clipped mean per amp to ensure each amp is on the same scale before doing the defect finding, so changing the flat scaling just changes the size of the offset subtracted by that clipped mean.
d321687
to
a923504
Compare
This removes the extraneous "exposure" because we do not want a new brighter fatter kernel for every exposure.
This avoids dimensionality clashes in existing repos; this will be fixed with DM-46439.
a923504
to
1a90571
Compare
This PR is the first implementation ticket for RFC-1044. The pipelines in this PR are not directly tested; the testing, in conjunction with the LATISS pipelines, is being done on DM-46356.