-
Notifications
You must be signed in to change notification settings - Fork 8
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-44150: Remove unnecessary DetectAndMeasure plugins #318
Conversation
4e48050
to
5704b96
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 fine, though I am slightly confused by the move of HSM defaults to dipoleFitTask
.
"ext_shapeHSM_HsmSourceMoments", | ||
"ext_shapeHSM_HsmPsfMoments", | ||
] | ||
self.measurement.slots.psfShape = "ext_shapeHSM_HsmPsfMoments" | ||
self.measurement.slots.shape = "ext_shapeHSM_HsmSourceMoments" |
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 am slightly confused by these being moved to dipoleFitTask
. Both seem fine, though setting here seems more general.
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.
It's a good question. dipoleFitTask
is our MeasurementTask
for diffim; it's what runs all of our measurement plugins. So I think it's the lowest level place to put the list of what measurements we consider to be canonical. I guess nothing in dipoleFitTask depends on the output of a shape algorithm, so we could just not define anything for the shape slot in that task and do it here (maybe with a note in dipoleFit to that effect?)? I could go either way.
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 thought about this more and I think you're right and I'll take it a step further: I'm going to move the plugins currently set in dipoleFitTask that are unnecessary for just that task out of it and into detectAndMeasure. That makes dipoleFit more self-contained, and puts the rest of the diffim measurements together in one place.
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.
Having poked at this some, it's not worth the added complexity of moving plugins out of dipoleFitTask, because of the various slots that have to be unset and then reset in detectAndMeasure.
We don't have a use for this flux measurement on diffim sources, which are either dipoles, psfs, or trails, none of which are elliptical Gaussians.
shapeHsm handles negative sources better than SdssShape, otherwise they're similar enough that there's no benefit here to running both.
This was carried over from much older code; we're now measuring on the difference exposure instead of the score exposure, so we don't need it.
This reverts commit 2fd0aeb2e2bd2ca630956e4bbcb27829650e56d0.
DipoleFitTask doesn't use shape measurements, and SdssShape doesn't handle negative sources well; detectAndMeasure uses shapeHsm, which is better.
5704b96
to
3b792af
Compare
No description provided.