DM-12540: Add HSM moments that use a circular weight function #16
Conversation
| @@ -37,7 +37,7 @@ class ImageConverter | |||
| // in the box and stride. | |||
| PixelT* ptr = reinterpret_cast<PixelT*>(array.getData() + (_box.getMinY() - _image->getY0())*stride + | |||
| _box.getMinX() - _image->getX0()); | |||
| return galsim::ImageView<PixelT>(ptr, _owner, stride, bounds); | |||
| return galsim::ImageView<PixelT>(ptr, _owner, 1, stride, bounds); | |||
There was a problem hiding this comment.
This is great, but presumably this has to be linked with an update to GalSim, which I don't see on this ticket.
There was a problem hiding this comment.
I was waiting for a tagged GalSim version which was tagged last night. I'll update the ticket.
config/enable.py
Outdated
| @@ -6,7 +6,7 @@ | |||
|
|
|||
| import lsst.meas.extensions.shapeHSM | |||
| config.plugins.names |= ["ext_shapeHSM_HsmShapeRegauss", "ext_shapeHSM_HsmSourceMoments", | |||
| "ext_shapeHSM_HsmPsfMoments"] | |||
| "ext_shapeHSM_HsmPsfMoments", "ext_shapeHSM_HsmRoundMoments",] | |||
There was a problem hiding this comment.
I'm not a fan of the naming choice: ext_shapeHSM_HsmRoundMoments in comparison with ...HsmSourceMoments and ...HsmPsfMoments suggests it's measuring the moments of a "round" (whatever that is). How about ...HsmSourceRoundMoments or ...HsmSourceMomentsRound?
There was a problem hiding this comment.
I guess I wasn't a fan of the original naming scheme, but I see your point. I think HsmSourceMomentsRound is the best description. However, I'm not a fan of the long names we have in the current catalogs, but it is probably best to be consistent.
| @@ -40,17 +40,34 @@ class HsmMomentsControl { | |||
| class HsmSourceMomentsControl { | |||
| public: | |||
| LSST_CONTROL_FIELD(badMaskPlanes, std::vector<std::string>, "Mask planes used to reject bad pixels."); | |||
| LSST_CONTROL_FIELD(roundMoments, bool, "Use round weight function."); | |||
| LSST_CONTROL_FIELD(addFlux, bool, "Store measured flux."); | |||
There was a problem hiding this comment.
I think that if you're going to call this a fundamentally different source measurement algorithm, the roundMoments setting shouldn't be in the HsmSourceMomentsControl (which leaks out to the user) but in HsmMomentsAlgorithm.
There was a problem hiding this comment.
It is actually the same algorithm in galsim, just with different parameters. I wanted this to be reflected in the LSST version. But we want to have the results of both algorithms in the output catalogs. So an alternative would be to create a second copy of HsmSrcMoments in the python layer and then set the appropriate parameters in a config file. However, people could potentially call the algorithm without setting the parameters and get incorrect results.
|
|
||
| HsmSourceMomentsControl(HsmSourceMomentsControl const & other) : | ||
| badMaskPlanes(other.badMaskPlanes) {} | ||
| badMaskPlanes(other.badMaskPlanes),roundMoments(other.roundMoments),addFlux(other.addFlux) {} |
There was a problem hiding this comment.
Spaces after commas.
| @@ -40,17 +40,34 @@ class HsmMomentsControl { | |||
| class HsmSourceMomentsControl { | |||
| public: | |||
| LSST_CONTROL_FIELD(badMaskPlanes, std::vector<std::string>, "Mask planes used to reject bad pixels."); | |||
| LSST_CONTROL_FIELD(roundMoments, bool, "Use round weight function."); | |||
| LSST_CONTROL_FIELD(addFlux, bool, "Store measured flux."); | |||
There was a problem hiding this comment.
I think the help strings should end with question marks rather than periods.
src/HsmMoments.cc
Outdated
| exposure.getMaskedImage().getMask(), | ||
| bbox, center, badPixelMask, 2.5*psfSigma); | ||
| exposure.getMaskedImage().getMask(), | ||
| bbox, center, badPixelMask, 2.5*psfSigma, _ctrl.roundMoments, _ctrl.addFlux); |
There was a problem hiding this comment.
I think this line is a tad longer than the limit.
tests/testHsm.py
Outdated
| @@ -134,6 +134,13 @@ | |||
| [58.5008586442, 28.2850942049], | |||
| ]) | |||
|
|
|||
| round_moments_expected = np.array([# sigma, e1, e2, flux, x, y | |||
There was a problem hiding this comment.
flake8 prolly wants you to have a couple of spaces before the #.
tests/testHsm.py
Outdated
| @@ -306,6 +313,31 @@ def testHsmSourceMoments(self): | |||
| self.assertAlmostEqual(xy, expected.getIxy(), SHAPE_DECIMALS) | |||
| self.assertAlmostEqual(yy, expected.getIyy(), SHAPE_DECIMALS) | |||
|
|
|||
| def testHsmRoundMoments(self): | |||
| for (i, imageid) in enumerate(file_indices): | |||
| source = self.runMeasurement("ext_shapeHSM_HsmRoundMoments", imageid, x_centroid[i], y_centroid[i], | |||
There was a problem hiding this comment.
I think this slightly exceeds the line length limit.
tests/testHsm.py
Outdated
| xy = source.get("ext_shapeHSM_HsmRoundMoments_xy") | ||
| flux = source.get("ext_shapeHSM_HsmRoundMoments_Flux") | ||
|
|
||
| # Centroids from GalSim use the FITS lower-left corner of 1,1 |
There was a problem hiding this comment.
Shouldn't we correct for that in the algorithm, to present a unified set of coordinates in the catalog?
There was a problem hiding this comment.
No, the issue here is the difference in convention between LSST and galsim. By default, the bounds of an image are constructed in galsim with (1,1) in the lower-left corner while LSST uses (0,0). The precomputed galsim values we have in the test were measured using this default. If we wanted to change this, we could change the bounds of the LSST image that is created in the test to match galsim or recompute the galsim values, but I don't think it is worth the effort.
| [1.77995181084, 0.0416346564889, -0.143147706985, 534.59197998, 9.51994111869, 12.6250775205], | ||
| [1.46549296379, -0.0831127092242, -0.0628845766187, 348.294403076, 20.6242279632, 39.5941625731], | ||
| [1.64031589031, 0.0867517963052, 0.0940798297524, 793.374450684, 58.4728765002, 28.2686937854], | ||
| ]) |
There was a problem hiding this comment.
I think flake8 is going to want two blank lines between this and the following def.
The standard HSM adaptive moments code uses an elliptical Gaussian as the weight function. In some cases, it is beneficial to instead use a circular Gaussian.
rearmstr
force-pushed
the
tickets/DM-12540
branch
from
1316e08
to
67f3d1e
Compare
No description provided.