Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: meas_algorithms, meas_extensions_psfex
-
Labels:None
-
Story Points:2
-
Epic Link:
-
Sprint:DRP F16-3
-
Team:Data Release Production
Description
Sogo Mineo writes in HSC-1414:
See the following lines:
meas_algorithms/HSC-4.0.0/python/lsst/meas/algorithms/objectSizeStarSelector.py:466
in ObjectSizeStarSelector.selectStars():
if psfCandidate.getWidth() == 0:psfCandidate.setBorderWidth(self._borderWidth)psfCandidate.setWidth(self._kernelSize + 2*self._borderWidth)psfCandidate.setHeight(self._kernelSize + 2*self._borderWidth)In reduceFrames, these lines set the width of psfCandidate to be 21
for the first time the execution reaches there.When the first CCD image has been processed, the worker process
continues to process another CCD image, and the execution reaches
here again. This time, psfCandidate.getWidth() is 41, because
psfexPsfDeterminer has set it to be 41, and the value has been
retained because the width is a static member. And so, for the second
CCD image, the width of psfCandidate is not 21 but 41.Since psfCandidates are widened, stars positioned at edges of images
are rejected. It results in a smaller number of PSF candidates than expected.Only CCD images that are initially given to the worker processes
are processed with psfCandidate.getWidth() == 21. The other CCD images are
processed with psfCandidate.getWidth() == 41.
When the number of SMP cores changes, CCD images are processed with different
parameters.The change in the number of PSF candidates results in different Psf, a different
result of image repair, and different catalogs.
The line numbers are different on the LSST side because of refactoring (objectSizeStarSelector.py:466 has moved to starSelector.py:148), but the bug is still present. The main problem appears to be that the PsfCandidate elements are static, are being set in both the star selector and the PSF determiner and one of those is conditional on what the value is. I will investigate moving the static class variables to instance variables — the desired size appears to vary by context, so it shouldn't be a class variable.
Attachments
Issue Links
Activity
You're exactly right about how this works. The PsfexPsfDeterminer will later change the width and height; that's fine, so long as we set the width and height back; that's what this ticket does.
Paul Price
added a comment - You're exactly right about how this works. The PsfexPsfDeterminer will later change the width and height; that's fine, so long as we set the width and height back; that's what this ticket does. Merged to master.
Paul Price
added a comment - Merged to master. That isn't what I meant.
Even if the width and height are set consistently based on the config values, the PsfCandidate.getMaskedImage() call will return an image based on the current underlying static _width and _height, possibly not the same as when the original PsfCandidate and _image were created in the code this ticket modifies.
That seems like unpredictable behavior to me, but I trust that you know this code better than I do.
Perry Gee
added a comment - That isn't what I meant.
Even if the width and height are set consistently based on the config values, the PsfCandidate.getMaskedImage() call will return an image based on the current underlying static _width and _height, possibly not the same as when the original PsfCandidate and _image were created in the code this ticket modifies.
That seems like unpredictable behavior to me, but I trust that you know this code better than I do. Unfortunately that's how it works at the moment. However, all uses of the PsfCandidate are prefixed with code to set the desired width and height for that use, so I believe everything is about the best it can be with this fragile design.
Paul Price
added a comment - Unfortunately that's how it works at the moment. However, all uses of the PsfCandidate are prefixed with code to set the desired width and height for that use, so I believe everything is about the best it can be with this fragile design. 
Yes, the code is certainly not thread-safe, which is why I kicked it back to you. Wasn't sure if that was an issue or not.
I assume that you looked at the code enough to assure yourself that once the image is created, the psfCandidate won't ever call getWidth or getHeight again? I notice that if candidate.getMaskedImage() is called later on, and the underlying _width or _height has been altered, it looks like getMaskedImage(width, height) will end up making a new image with the new class static _width and _height.