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-15795: Create DiaForcedSourceTask #40
Conversation
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.
Looks good. My comments are mostly fairly minor documentation and formatting requests.
64 - expIdBits) | ||
idFactoryDirect = afwTable.IdFactory.makeSource( | ||
diffim.getInfo().getVisitInfo().getExposureId(), | ||
64 - expIdBits) |
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.
Could you use something like DM-17689 to avoid hard-coded numbers here?
tests/test_diaForcedSource.py
Outdated
n_points : `int` | ||
Number of DiaObject test points to create. | ||
wcs : `lsst.afw.geom.SkyWcs` | ||
Wcs to convert RA/Dec to x/y if provided. |
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.
What do you mean by "if provided" here? It doesn't look optional.
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.
Removed. I re-purposed a previous method.
tests/test_diaForcedSource.py
Outdated
# Make images with one source in them and distinct values and | ||
# variance for each image. | ||
# Direct Image | ||
ms = afwImage.MaskedImageF(lsst.geom.ExtentI(self.imageSize[0] + 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.
"ms" is not a very descriptive variable name. Is the "m" for masked image and the "s" for source? I'd prefer to see a slightly longer name, but I guess this is ok for a test.
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.
sorry, these were copy-pasted from a different unittest. I'll give them slightly more descriptive names.
tests/test_diaForcedSource.py
Outdated
lsst.geom.PointI(1, 1), | ||
lsst.geom.ExtentI(self.imageSize[0], | ||
self.imageSize[1])) | ||
self.mi = afwImage.MaskedImageF(ms, bbox, afwImage.LOCAL) |
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.
Does self.mi
need to be saved as a class attribute? You're just overwriting the value you set it to a few lines back, which suggests you don't need it outside of setting up self.exposure
and self.diffim
.
tests/test_diaForcedSource.py
Outdated
|
||
FWHM = 5 | ||
psf = measAlg.DoubleGaussianPsf( | ||
15, 15, FWHM/(2*np.sqrt(2*np.log(2)))) |
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 this could fit on one line.
Add DiaForcedSourceTask. Add bitpackin for direct and difference image flags. Simplify DiaForcedSourceTask. Simplify to DiaForcedSourceTask only.
4a39db4
to
03cc98f
Compare
Partially debuged unittest. Need to add footprints to measurement catalog. Partially debugged unittests. Add coord to centroid measurement plugin. Remove unnecessary code from inherited plugin class. Remove pdb statement. Fix linting Add test values. Create distict idFactories. Fix linting
Remove outdated docstring. Make better test variable names. Fix line spacing.
03cc98f
to
2bf19bc
Compare
No description provided.