New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DM-29673 Add centre of mass centroiding to support donut images #497
Conversation
4e0f2a4
to
35b348d
Compare
SdssCentroids fit donuts very poorly, and a simple CoM centroid provides a pretty robust measurement when seeded with the position using the existing algorithm. This is not expected to deal with overlapping donuts as it is used to simply to give a cutout to CWFS analysis.
35b348d
to
12195ba
Compare
6fac018
to
cf19de4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some quick comments. Don't call this a review because I haven't really looked at the algorithmic code.
774bc59
to
30d6b0d
Compare
|
||
result.brightestObjCentroidCofM = None | ||
try: | ||
boxSize = donutDiameter * 1.3 # allow some slack, as cutting off side of donut is very bad |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If missing the side is bad, should you check for that, or will you just get an exception and return None, indicating that there was a problem? I'm also not clear what would cause this block to raise. _getCenterOfMass doesn't directly raise, and I don't see why anything else would raise either. Am I just missing something obvious>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not that bad, it just means the result won't be very good, and checking for it is definitely not easy (all of which we discussed OOB, but just noting here too). Also, downstream code with see that, and it's not a huge problem anyway (plus that code is responsible for passing the boxsize anyway).
You're right about the try
, and I've removed it completely now (having run some tests to make sure I'm definitely OK if the boxsize goes off the edge of the chip).
SdssCentroids fit donuts very poorly, and a simple CoM
centroid provides a pretty robust measurement when seeded
with the position using the existing algorithm.
This is not expected to deal with overlapping donuts as it
is used to simply to give a cutout to CWFS analysis.