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-43045: Stand-up per-camera cpSky configurations #236
Conversation
python/lsst/cp/pipe/cpSkyTask.py
Outdated
self.largeScaleBackground.xSize = 122.88 # in mm | ||
self.largeScaleBackground.ySize = 122.88 # in mm | ||
self.largeScaleBackground.pixelSize = 0.015 # in mm per pixel | ||
# These values correspond to the HSC recommendation. | ||
self.largeScaleBackground.minFrac = 0.1 | ||
self.largeScaleBackground.mask = ['BAD', 'SAT', 'INTRP', 'DETECTED', 'DETECTED_NEGATIVE', |
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.
If preferred, the minFrac
and mask
configs could also be moved out of the task and into a pipelines _ingredients
YAML, which would allow us to retire the setDefaults
method here entirely. I haven't done that at this time, but I'm happy to do so if that reads better and would be better for long-term maintainability.
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.
As these remaining fields are the same as the original config values, we can drop them here, as well as this method.
ee24036
to
549aeda
Compare
549aeda
to
a5c3258
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.
A few suggestions in comments.
python/lsst/cp/pipe/cpSkyTask.py
Outdated
self.largeScaleBackground.xSize = 122.88 # in mm | ||
self.largeScaleBackground.ySize = 122.88 # in mm | ||
self.largeScaleBackground.pixelSize = 0.015 # in mm per pixel | ||
# These values correspond to the HSC recommendation. | ||
self.largeScaleBackground.minFrac = 0.1 | ||
self.largeScaleBackground.mask = ['BAD', 'SAT', 'INTRP', 'DETECTED', 'DETECTED_NEGATIVE', |
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.
As these remaining fields are the same as the original config values, we can drop them here, as well as this method.
pipelines/Latiss/cpSky.yaml
Outdated
# These values roughly split a single raft into 4x4 superpixels. | ||
# As noted below, the sizes are in millimeters, and correspond to a | ||
# background image of 6144*6144 pixels (6144*0.01=61.44). | ||
largeScaleBackground.xSize: 61.44 # in mm |
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.
If these are known not to work (as shown by the figures on the ticket), then we should file a ticket to figure out correct values, and reference that ticket here and below (for just the cameras that need more development work).
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.
But this one shouldn't be larger than the available pixels, probably.
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've filed ticket DM-43321 to capture future effort on this, and left a note in each non-HSC YAML referencing this ticket.
All 6k superpixel configs have been reverted to their prior 4k dimensions on this ticket.
These configs are the primary task defaults, so are not required to be overridden here.
a5c3258
to
e3fa6a3
Compare
No description provided.