tickets/DM-12431: re-Verify performance of matchPessimisticB with new distortions. #79
Conversation
Changed behavior of final output matches to the smaller of a sigma clipped cut and the current maxMatchDistance.
Added 2 sigma clipping to final dist max cut. Added return case if not match is found. Modified config in matchPessimistic test.
| (self.config.minMatchDistPixels * wcs.pixelScale().asArcseconds(), | ||
| maxMatchDistArcSec) | ||
| ) | ||
| dist_cut_arcsec = np.min((clip_max_dist, maxMatchDistArcSec)) |
There was a problem hiding this comment.
Seems like this makes the comment above untrue, right? You are no longer picking the largest of anything, but the smallest of the sigma clipped cut and maxMatchDistArcSec.
| dist_cut_arcsec = np.min((clip_max_dist, maxMatchDistArcSec)) | ||
| if not np.isfinite(clip_max_dist): | ||
| clip_max_dist = maxMatchDistArcSec | ||
|
|
There was a problem hiding this comment.
There are eight characters here that you don't want.
|
Seems like the second commit ought to be a couple of separate commits really. |
| # The matcher returns all the nearest neighbors that agree between | ||
| # the reference and source catalog. For the current astrometric solver | ||
| # we need to remove as many false positives as possible before sending | ||
| # the matches off to the solver. | ||
| distances_arcsec = np.degrees(matcher_struct.distances_rad) * 3600 | ||
| distances_arcsec = np.degrees(matcher_struct.distances_rad) * 3600 |
There was a problem hiding this comment.
You added extra whitespace that you don't want to this line.
| minMatchedPairs: | ||
| dist_cut_arcsec = maxMatchDistArcSec | ||
| else: | ||
| dist_cut_arcsec = np.min((clip_max_dist, maxMatchDistArcSec)) |
There was a problem hiding this comment.
These changes make the comment above untrue — you are not picking the largest of anything, but the minimum of clip_max_dist and maxMatchDistArcSec (except when you aren't).
There was a problem hiding this comment.
The comment has been edited.
| distances_arcsec = np.degrees(matcher_struct.distances_rad) * 3600 | ||
| distances_arcsec = np.degrees(matcher_struct.distances_rad) * 3600 | ||
| clip_max_dist = np.max( | ||
| (sigmaclip(distances_arcsec, low=100, high=2)[-1], |
There was a problem hiding this comment.
What's the justification for these values of low and high? They just seem like magic numbers here.
There was a problem hiding this comment.
The are the number of sigma for the clip. Low is 100 because the lower bound is always a value of 0 and high is a 2 sigma cut. I will add a comment explaining this. I could also make the high value a configurable if that is useful.
| match_tolerance.lastMatchedPattern = None | ||
| maxShiftArcseconds = \ | ||
| self.config.maxOffsetPix * wcs.pixelScale().asArcseconds() | ||
| continue |
There was a problem hiding this comment.
What does this continue buy you?
There was a problem hiding this comment.
You correct, it buys me nothing really. Just habit. It is now removed.
| matcher_struct.shift * afwgeom.arcseconds | ||
| match_tolerance.lastMatchedPattern = \ | ||
| matcher_struct.pattern_idx | ||
| for soften_idx in range(self.config.matcherIterations): |
There was a problem hiding this comment.
Could you maybe clean up the language around here a bit please?
Specifically, both soften_idx and try_idx are "softening", they're just softening different things. Maybe clearer to use, say, soften_pattern and soften_dist, or something like that.
Remove whitespace. Reword and add comments around distance selection. Change pattern and distance softenig names.
Added specific camera and visits this criteria was tested on.
Changed try_idxs to new variable names.
Changed behavior of final output matches to the smaller of
a sigma clipped cut and the current maxMatchDistance.
This change adds a bit more stability to output match quality and fit/match cycle decent.