DM-17098: fix SkyMeasurementTask.measureScale #67
Conversation
| for xIndex, yIndex in itertools.product(range(self.config.xNumSamples), | ||
| range(self.config.yNumSamples)): | ||
| xStart, xStop = xLimits[xIndex], xLimits[xIndex + 1] | ||
| yStart, yStop = yLimits[yIndex], xLimits[yIndex + 1] |
There was a problem hiding this comment.
Do you care that all your boxes will not be the same size? Or is it just important that they are all done. Also you should put in some kind of check to make sure that self.config.(x/y)NumSamples is not greater than image.get(widht/height) or you will be sampling the same sub image over multiple times in places.
There was a problem hiding this comment.
The limits vectors (xLimits and yLimits) are set using np.linspace, and we're iterating over indices for those vectors. This means:
- the boxes are all (roughly) the same size (only "roughly" because the limits are integerised, so there'll be differences of up to a pixel in each dimension, but that's not a big deal)
- the iteration isn't going to run off the end of the image (the limits vectors have a length of
[xy]numSamples + 1, we're iterating over[xy]NumSamplesindices and retrieving theiandi+1values in each iteration, so the length and the iteration match).
Just spotted and fixed a typo.
There was a problem hiding this comment.
I was specifically worried about something like this...
In [7]: xNumSamples = 15
In [8]: yNumSamples = 15
In [9]: width = 10
In [10]: height = 10
In [11]: xLimits = np.linspace(0, width-1, xNumSamples+1, dtype=int)
In [12]: yLimits = np.linspace(0, height-1, yNumSamples+1, dtype=int)
In [13]: yLimits
Out[13]: array([0, 0, 1, 1, 2, 3, 3, 4, 4, 5, 6, 6, 7, 7, 8, 9])
And so if you do this in a product, you are going to get all kinds of regions repeated.
SkyMeasurementTask.measureScale uses zip to iterate over x and y limits, so it's only iterating over the diagonal of the image. Using itertools.product, we iterate over the entire image.
PaulPrice
force-pushed
the
tickets/DM-17098
branch
from
59900dd
to
0e355d9
Compare
| @@ -276,15 +276,20 @@ def measureScale(self, image, skyBackground): | |||
| """ | |||
| if isinstance(image, afwImage.Exposure): | |||
| image = image.getMaskedImage() | |||
| xLimits = numpy.linspace(0, image.getWidth() - 1, self.config.xNumSamples + 1, dtype=int) | |||
| yLimits = numpy.linspace(0, image.getHeight() - 1, self.config.yNumSamples + 1, dtype=int) | |||
| xNumSamples = min(self.config.xNumSamples, image.getWidth()) | |||
There was a problem hiding this comment.
Maybe add a comment here about why you are taking the minimum, in case a future editor of the code is confused
Nate Lust encouraged me to consider what happens when the number of samples is larger than the image, which identified some off-by-one issues. The limits now run to the edge of the image, and we subtract one from the bounding box end point because that is inclusive.
PaulPrice
force-pushed
the
tickets/DM-17098
branch
from
be663fa
to
a03763e
Compare
SkyMeasurementTask.measureScale uses zip to iterate over x and y
limits, so it's only iterating over the diagonal of the image.
Using itertools.product, we iterate over the entire image.