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-4550 Degenerate Templates #13
Conversation
log.trace('Reweighting templates') | ||
nchild = np.sum([pkres.skip is False for pkres in res.peaks]) | ||
A = np.zeros((W*H, nchild)) | ||
pimage = afwImage.ImageF(bb) |
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.
Any particular reason for calling this "pimage"?
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.
Yes, this is the parent image. I will rename it parentImage
while True: | ||
|
||
if weightTemplates: | ||
# Reweight the templates by doing a least-squares fit to the image |
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.
This is WAY more readable than the previous (and broken!) version :)
for pkres in res.peaks: | ||
if pkres.skip: | ||
continue | ||
cimage = afwImage.ImageF(bb) |
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.
Ditto for "cimage"?
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.
Will change to childImage
cimage = afwImage.ImageF(bb) | ||
afwDet.copyWithinFootprintImage(fp, pkres.templateImage, cimage) | ||
A[:, index] = cimage.getArray().ravel() | ||
index +=1 |
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.
Space after +=
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.
Fixed.
pkres.setTemplateWeight(X1[index]) | ||
index += 1 | ||
|
||
|
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.
Superfluous blank line.
for pk in fp.getPeaks(): | ||
print(' ', pk.getIx(), pk.getIy()) | ||
|
||
|
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.
Superfluous newline.
self.assertTrue(deb.peaks[3].degenerate) | ||
self.assertTrue(deb.peaks[4].degenerate) | ||
self.assertTrue(deb.peaks[5].degenerate) | ||
|
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.
Again, I'm not sure how you can be absolutely sure that these are the indices the degenerate will live in (if, for example, 2 or 4 peaks were detected)
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.
Because I implicitly put the last three peaks into the list by hand. The deblending code does not change the order of peaks and all have lower flux than the actual peaks from the images.
|
||
if __name__ == "__main__": | ||
lsst.utils.tests.init() | ||
unittest.main() |
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 seems to me there could be quite a bit more test coverage. Given the state it was in before this ticket, the weightTemplates
option should have a unittest. Also, the correct behavior in the template removal could be tested (i.e. criteria based on deblendedAsPsf
&/or maximumTemplate
). If you think this is out of scope, I'm happy for just an "Expand test coverage" ticket to be created.
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.
I'm not too concerned about testing deblendedAsPsf because I'm not really sure that this is doing a good job. I think it should have a test of its own if we really want to use it. I have already seen cases in HSC where it is obviously wrong. It was in the SDSS deblender, but maybe it should be removed.
I do think weightTemplates needs to have a unit test. I can add this as an additional ticket.
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.
I think we will want to keep the concept, although the implementation may need to be totally redone. As we get further down towards the plane almost every object will be a PSF, and I'm thinking about using a prior on deblendedAsPsf
as a way to transition to a classic crowded field code in regions of very high stellar density.
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.
I agree that the concept is useful, I'm just not sure how helpful it is in identifying and removing degenerate templates.
weightTemplates = pexConf.Field(dtype=bool, default=False, | ||
doc=("If true, a least-squares fit of the templates will be done")) | ||
removeDegenerateTemplates = pexConf.Field(dtype=bool, default=False, | ||
doc=("Try to remove similar tempates?")) |
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.
Typo -> temp*l*ates
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.
Fixed
reject = indexes[i] | ||
elif res.peaks[keep].deblendedAsPsf is False and res.peaks[reject].deblendedAsPsf: | ||
reject = indexes[rejectedIndex] | ||
keep = indexes[i] |
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.
This elif
is sort of redundant against the initial setting in lines 493/494, but I guess it makes the intent clearer, so feel free to leave it (and I suppose the else would then need a check that both peaks have the same deblendedAsPsf
values).
The ability to reweight the templates by doing a least-squares fit did not work and could not be called from the SourceDeblendTask.
4e9d871
to
f5ca2ab
Compare
Compute the dot product between all pairs of templates and remove one of the objects if the dot product is larger than threshold.
f5ca2ab
to
4c66146
Compare
No description provided.