DM-42981: Convert AstrometryTask to new exception handling system #190
Conversation
This was useful in the past as parts of the Exposure API were still in flux, but it's stable enough now that this shouldn't be necessary.
This snake_case stuck out, so I fixed it.
parejkoj
force-pushed
the
tickets/DM-42981
branch
3 times, most recently
from
ebc140e
to
34449e3
Compare
| result = pipeBase.Struct(matchTolerance=None) | ||
| maxMatchDistance = np.inf | ||
| i = 0 | ||
| while (maxMatchDistance > self.config.minMatchDistanceArcSec and i < self.config.maxIter): |
There was a problem hiding this comment.
I'm surprised we don't consider it an error (or at least some sort of qualified success) if we hit the maximum number of iterations without satisfying the max distance criteria. If we add that, then I think the old form of the loop may be closer to what we want; it's a very natural for-else:
for i in range(self.config.maxIter):
... # do fit
if maxMatchDistance <= self.config.minMatchDistanceArcSec:
# unqualified success!
break
else:
self.log.warning("Maximum iterations exceeded...")
Of course, for-else isn't exactly super familiar to people, and that makes me wonder if wrapping the whole loop up in a separate method that can return or raise an already-updated exception would be even better.
There was a problem hiding this comment.
I like for-else in general, but I'm not sure it's right here. I read minMatchDistanceArcSec as "definitely stop here, that's plenty good", and not a quality threshold. But also, on DM-44012, I'm going to look at whether this outer loop is even necessary at all: I believe that with the pessimistic matcher and the affine fitter, there's no need for this outer loop (and the ticket it was added on, DM-2755, has an argument between Paul and Russell about whether we should even do that loop. We now have a much better matcher, so the original rational mostly doesn't exist.
AstrometryTask has grown organically without an overarching vision, and I think there are too many different kinds of stop/quality criteria. That's a bigger question than this ticket, though.
There was a problem hiding this comment.
Ok, fine to leave this as-is.
| @@ -421,6 +394,8 @@ def _matchAndFitWcs(self, refCat, sourceCat, goodSourceCat, refFluxField, bbox, | |||
| initWcs=wcs, | |||
| bbox=bbox, | |||
| refCat=refCat, | |||
| # TODO: why is this not `goodSourceCat`? | |||
| # Answer: because `fitWcs` calls `updateSourceCoords`: should it? | |||
There was a problem hiding this comment.
Probably not, but I think you have enough scope on this ticket already.
There was a problem hiding this comment.
This is one of the things I hope to dig into on DM-44012.
| msg : `str` | ||
| Informative message about the nature of the error. | ||
| kwargs | ||
| All other arguments are added to a ``_metadata`` attribute, which is |
There was a problem hiding this comment.
Feels a little strange to have the private attribute's name included in the docs like this. Is the name not actually an implementation detail?
There was a problem hiding this comment.
I'm torn on that: I directly modify _metadata in AstrometryTask to add more information to exceptions that came from the matcher or fitter. I could name it something non-"private", but metadata (the read-only property) is the most obvious, but taken, choice.
There was a problem hiding this comment.
I think I see that it's effectively "protected" rather than just "private", and those are always in a nebulous state in Python already. Fine to leave it as-is.
* Rework loop to simplify logic. * Remove "catch all exceptions" logic, replacing with just catching AstrometryError (which could be raised by the fitter or matcher). * Remove "using previous iteration" which appears to be a hack added on DM-2755 to deal with problems they were having with the old matcher. * Remove/rename variables to hopefully reduce excessive indirection. * Expand tests to reflect the new exception logic. * Remove testWcsFailure that was based on "never raise" approach.
* Remove testUsedFlag and incorporate its test of calib_astrometry_used into the all the `doTest` calls. * Removed unneeded tearDown. * Use more standard `task` instead of `solver`.
It is not used elsewhere, so no need for it until we're done.
Previously, AstrometryTask would pass if it just kept the WCS on the exposure the same: the test now starts with a "wrong" (undistorted) WCS and will fail if the output WCS matches the input (at least for the testRadial case).
parejkoj
force-pushed
the
tickets/DM-42981
branch
from
34449e3
to
b830d46
Compare
No description provided.