DM-53170: Speed up the diffraction spike mask in crowded fields #1178
Conversation
laurenam
left a comment
laurenam
left a comment
There was a problem hiding this comment.
Just a few comments/questions. Also, please have a relook at test_diffractionSpikeMask.py to make sure you think this is effectively/adequately getting probed there.
Also, I think this assumes the diffraction is always aligned with the detector. Is that a save assumption in general?
| distance : `float` | ||
| The distance by which the point is outside the box, or 0 if it is | ||
| inside. | ||
| """ |
There was a problem hiding this comment.
Might be worth mentioning units (bbox pixels) in the docstrings?
| x1, y1 = bbox.getEnd() | ||
| dx = 0 if x0 <= x <= x1 else min(abs(x0 - x), abs(x - x1)) | ||
| dy = 0 if y0 <= y <= y1 else min(abs(y0 - y), abs(y - y1)) | ||
| return max(dx, dy) |
There was a problem hiding this comment.
Are you sure you want max here?
There was a problem hiding this comment.
Yes, if the source is too far away in either x or y, the diffraction spike will miss the image.
| return refCat[bright][spikeCandidates].copy(deep=True) | ||
|
|
||
| def selectSources(self, xvals, yvals, mask): | ||
| def selectSources(self, xvals, yvals, mask, radii): |
There was a problem hiding this comment.
Need to add radii to the Parameters list (especially as the name is really not an unambiguous descriptor for what it represents...is it really something like brightStarMaskRadii?)
There was a problem hiding this comment.
Good point, I'll change the name.
| else: | ||
| candidates[i] = True | ||
| # If the candidate bright source is off the image, only make a | ||
| # model for it if the diffraction spikes are long enough |
There was a problem hiding this comment.
Period at end of sentence.
The alignment is controlled by the |
isullivan
force-pushed
the
tickets/DM-53170
branch
from
32f701e to
9028be9
Compare
|
Good call on the unit tests! I spent the afternoon writing a new unit test, and it uncovered a subtle bug. |
No description provided.