meas_base review for DM-4926: centroids outside Footprints. #43
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -201,6 +201,42 @@ class CentroidTransform : public BaseTransform { | |||
| afw::table::CovarianceMatrixKey<ErrElement,2> _coordErrKey; | |||
| }; | |||
|
|
|||
| class CentroidChecker { | |||
|
|
|||
There was a problem hiding this comment.
Quibble: this newline is unnecessary.
|
It wasn't introduced on this issue, but since I've noticed it here, could you fix the "instnace" type in the |
|
|
||
| # This is a measure routine which does nothing except to raise Exceptions | ||
| # as requested by the caller. Errors normally don't occur unless there is | ||
| # something wrong in the inputs, or if there is an error during the measurement |
There was a problem hiding this comment.
I find "as requested by the caller" a bit confusing here. do you mean "Exceptions caused by moving the true result the amount specified in the config"?
There was a problem hiding this comment.
This comment is no longer relevant. It was for a previous incarnation.
pgee2000
force-pushed
the
tickets/DM-4926
branch
6 times, most recently
from
6293fd7
to
48d5d64
Compare
The CentroidChecker should be used by a Centroid Plugin. It fixes centroider errors whose result is outside the footprint. It must be constructed during __init__, as it adds a flag_resetToPeak to the subschema of the centroid plugin. The centroidChecker(measRecord) should be called after a plugin stores it measured centroid in its measRecord. If the centroid found lies outside the footprint, the result will be "reset" to the footprint peak. If maxDistFromPeak > 0 in the config of the plugin, a check and reset is also done for centroids which fall further than specified from the footprint peak. When a reset if done, flag and flag_resetToPeak are set. Unit test for CentroidChecker.py including toy plugin to simulate errors. Also check that the plugin works with the base Centroid algorithms. Naive, Gaussian and SdssCentroid changed to use checker.
I think this is leftover from an older style of setting flags, which allowed the algorithm to set flags to True, then throw out from anywhere. Such failures will now be caught by the framework. Plus, this strategy would circumvent any other code (such as the Centroid Checker itself) which might set a failure flag but allow the plugin to continue on.
pgee2000
force-pushed
the
tickets/DM-4926
branch
from
48d5d64
to
a2db85c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.