DM-33446: Use TraceRadius in GAaP plugin #23
Conversation
| if not (psfShape.getDeterminantRadius() > 0): | ||
| self.log.debug("PSF shape has non-positive determinant radius at (%f, %f)", center.x, center.y) |
There was a problem hiding this comment.
This ticket changes psfShape to use getTraceRadius always. If so, why is this debug log message relating to getDeterminantRadius required? Is the determinant radius used elsewhere, where this logger might be useful, or could it safely be removed?
There was a problem hiding this comment.
This can be safely removed. I had put that to indicate that the PSF at that location was bad, but I don't think gaap plugin is the place to put that.
| self.log.debug("PSF shape has non-positive determinant radius at (%f, %f)", center.x, center.y) | ||
|
|
||
| psfSigma = psfShape.getTraceRadius() | ||
| if not (psfSigma > 0): # This captures nan |
There was a problem hiding this comment.
If all you want to do here is capture NaNs, wouldn't np.isnan be better? Or do you expect some negative values of getTraceRadius to creep through for other reasons? If so, the in-line comment should reflect that possibility.
There was a problem hiding this comment.
I don't think either of getTraceRadius or getDeterminantRadius will ever return negative numbers, but in the off-chance they returned -1 or something, this will capture that. If I remember right, when I wrote this line, I didn't want to have to import numpy just for this condition. The way I read it, if the PSF size is not a positive-valued number, then don't proceed. Do you think np.isnan makes the intention much clearer?
There was a problem hiding this comment.
If there is a chance that a negative number may be returned, then yes, it makes sense to leave it as-is. I would however recommend changing the in-line comment to something like This captures NaN or negative values.
| for scalingFactor in self.config.scalingFactor} | ||
| raise GaapConvolutionError(errorCollection) | ||
| else: | ||
| errorCollection = {} |
There was a problem hiding this comment.
This may be personal preference, but I think the syntax you used before (dict()) reads better than the suggested change ({}). Completely up to you however!
arunkannawadi
force-pushed
the
tickets/DM-33446
branch
from
848de99
to
ce1c031
Compare
arunkannawadi
force-pushed
the
tickets/DM-33446
branch
from
ce1c031
to
adfaec9
Compare
Replace
getDeterminantRadiuscalls withgetTraceRadius