DM-40507: Cleanup DipoleFitTask #305
Conversation
The loops made it hard to find where these keys were created, and there are helpers to create flux and centroid keys with their associated uncertainties automatically. Using NO_UNCERTAINTY for centroids, to make it explicit that we don't have uncertainties for these values right now.
* Naive* are purely examples and shouldn't be used in production * Peak* are unnecessary with Psf and Dipole models and mostly for testing purposes * PsfDipoleFlux is slower than PsfDipoleFit (this task). * PsfDipoleFit provides a dipole classification.
PsfDipoleFlux is the older, slower algorithm, but it makes a useful check on the new PsfDipoleFit.
parejkoj
force-pushed
the
tickets/DM-40507
branch
from
bbc52cc
to
8cc82d0
Compare
| self.doReplaceWithNoise = False | ||
|
|
||
|
|
||
| class DipoleFitTask(measBase.SingleFrameMeasurementTask): | ||
| """A task that fits a dipole to a difference image, with an optional separate detection image. | ||
| """A task that fits a dipole to a difference image, with an optional | ||
| separate detection image. |
There was a problem hiding this comment.
I know you are just reformatting this description, but I don't see where we support an optional separate detection image.
There was a problem hiding this comment.
The posExp/negExp kwargs are what give this support.
parejkoj
force-pushed
the
tickets/DM-40507
branch
from
dd61a0e
to
7e452ec
Compare
| @@ -1113,6 +1113,7 @@ def measureDipoles(self, measRecord, exposure, posExp=None, negExp=None): | |||
| ): | |||
| measRecord.set(self.classificationFlagKey, False) | |||
| measRecord.set(self.classificationAttemptedFlagKey, False) | |||
| # TODO: Do we want to fail here, or just leave it marked as not fit? | |||
There was a problem hiding this comment.
@isullivan : this question still needs answering: I would lean towards not "failing" them, and just leaving them marked as not being attempted to be fit nor as dipoles.
parejkoj
force-pushed
the
tickets/DM-40507
branch
from
7e452ec
to
8bfe439
Compare
| plt.subplot(1, 3, 1) | ||
| display2dArray(z, 'Data', extent=extent) | ||
| plt.subplot(1, 3, 2) | ||
| display2dArray(fit, 'Model', extent=extent) | ||
| plt.subplot(1, 3, 3) | ||
| display2dArray(z - fit, 'Residual', extent=extent) | ||
| return fig |
There was a problem hiding this comment.
Why remove fig from the return? Note that it is still documented as the return value. Also the function display2dArray does still return fig, but that isn't used.
There was a problem hiding this comment.
Because it was causing the show down below to not get used, and it wasn't used by anything. I've removed it from the docstring.
| # Peaks are the same sign (not a dipole) | ||
| or (len(pks) > 1 and (np.sign(pks[0].getPeakValue()) | ||
| or (nPeaks > 1 and (np.sign(pks[0].getPeakValue()) |
There was a problem hiding this comment.
Are the peaks guaranteed to be ordered from largest positive peak to largest negative peak? I am concerned about checking pks[0] and pks[-1] explicitly, instead of sorting the peaks. If the peaks are sorted, please add a code comment stating that assumption.
There was a problem hiding this comment.
Yes, our convention is that peaks are always ordered from highest to lowest. This is a pretty deep convention and a lot of code depends on it.
| measRecord[self.posFluxErrKey] = result.signalToNoise # to be changed to actual sigma! | ||
| measRecord[self.posCentroidKeyX] = result.posCentroidX | ||
| measRecord[self.posCentroidKeyY] = result.posCentroidY | ||
| measRecord[self.posFluxKey.getInstFlux()] = result.posFlux |
There was a problem hiding this comment.
I didn't realize this was how we needed to set by key!
There was a problem hiding this comment.
This is one way. I think when this was written, the string-based method (measRecord["blah_instFlux"]) was much slower, which has since been mostly fixed. But having the keys in the plugin makes it easier, since you can just get the field key directly.
parejkoj
force-pushed
the
tickets/DM-40507
branch
2 times, most recently
from
9e52b8b
to
229a75a
Compare
* Enforce plugin order, so that DipoleFit can fall back on SdssCentroid for non-dipoles. * DipoleFitPlugin can run at "centroid" order, because it simultaneously fits centroids and fluxes. * Switch centroid slot after SdssCentroid, so that the "best" centroid comes from DipoleFit (even if it's just copied over). * Switch dipole centroid field names to better match centroid slot convention (foo_x/foo_y, for plugin foo). * Rename DipoleFitTask default name to match Task name convention. Cleanup tests to pass with the better centroids: * Loosen flux tolerance. * Remove test of "unphysical" sources that relied on bad centroiding pushing the sky sources off the image. * Remove tests that relied on old centroider behavior when measuring on unmerged footprints. Refactor non-dipole "error" handling: * No need to flag sources that were not fit as failures, they're already marked as not dipoles. * Handle "too large" footprints as failures separately from other non- dipole sources. * Remove redundant flag setting: some flags either default to False, or were set just above.
We want the debug logs to appear if the loglevel is DEBUG. Plot display was broken because `show()` never got called!
parejkoj
force-pushed
the
tickets/DM-40507
branch
from
229a75a
to
5a0dcae
Compare
No description provided.