DM-41994: Use galsim primitives vs higher level interface. #48

erykoff · 2023-12-04T17:26:36Z

No description provided.

python/lsst/meas/extensions/shapeHSM/_hsm_moments.py

rmjarvis

I wouldn't recommend going this way. This is using implementation details in ways that are basically equivalent to the C++ syntax reliance that you currently have in the stack. I assume most of the reason for this is to avoid the heavy overhead in the galsim.Image constructor.

However, you should be able to remove most of that using the supported galsim._Image factory function. It's a leading underscore function, but it is documented, which means it is guaranteed to be API stable using the normal semantic versioning rules. cf. http://galsim-developers.github.io/GalSim/_build/html/image_class.html#galsim._Image

I think using that would get you most of the performance you're looking for here without the fragility of relying on implementation details.

rmjarvis · 2023-12-05T01:07:48Z

python/lsst/meas/extensions/shapeHSM/_hsm_moments.py

+        array.strides[1]//array.itemsize,
+        array.strides[0]//array.itemsize,
+        bounds._b,
+    )


Is this actually faster than using galsim._Image(array, bounds, None)? That's the supported API that skips most of the sanity checks. This interface isn't guaranteed to be stable across versions.

Thanks! I didn't know about this, and will take a look. It does look like it should do what we need, and will confirm.

rmjarvis · 2023-12-05T03:35:20Z

python/lsst/meas/extensions/shapeHSM/_hsm_moments.py

+            shape = galsim.hsm.ShapeData()
+            hsmparams = galsim.hsm.HSMParams.default
+
+            galsim._galsim.FindAdaptiveMomView(


Even this isn't really guaranteed to be stable, but it should be very close to equivalent to image.FindAdaptiveMom(...).

So there was actually a significant amount of time spent in _convertMask which was some of the motivation to use the primitive. If there was an officially supported way of bypassing this I think we could use the official API.

Oh right. The direct Python API calls the _convertMask function which has a bunch of galsim.Image instantiations making it slow as well. I could open an issue on the GalSim repo if there's no immediate supported API.

That's fair. What mask do you use? None? Or something real?

We supply a value for badpix that masks some pixels, so very much real. I think the overheads there can be reduced by using galsim._ImageI instead of galsim.ImageI in _convertMask.

GalSim-developers/GalSim#1264

Right now, this is the only sticking point I believe. Either we use the FindAdaptiveMom call or have a TODO comment here referring to a ticket that should replace it with FindAdaptiveMom as soon as we have a version of GalSim with the above issue implemented.

rmjarvis · 2023-12-05T03:36:03Z

python/lsst/meas/extensions/shapeHSM/_hsm_moments.py

@@ -278,19 +292,20 @@ def measure(self, record, exposure):
        # Create a GalSim image using the extracted array.
        # NOTE: GalSim's HSM uses the FITS convention of 1,1 for the
        # lower-left corner.
-        image = galsim.Image(imageArray, bounds=bounds, copy=False)
+        _image = _quickGalsimImageView(imageArray, bounds)


Here, I would recommend image = galsim._Image(imageArray, bounds, None).

rmjarvis · 2023-12-05T03:38:52Z

python/lsst/meas/extensions/shapeHSM/_hsm_moments.py

@@ -430,7 +445,17 @@ def measure(self, record, exposure):
        imageArray = psfImage.array

        # Create a GalSim image using the PSF image array.
-        image = galsim.Image(imageArray, bounds=bounds, copy=False)
+        _image = _quickGalsimImageView(imageArray, bounds)


Also here. image = galsim._Image(imageArray, bounds, None).

arunkannawadi · 2023-12-05T03:43:05Z

@rmjarvis recommends using galsim._Image to avoid the overhead instead of the APIs that aren’t guaranteed to be stable. In hindsight, I should have known it but completely forgot about it. That should be the best alternative to _quickGalsimImageView. We don’t gain much by using FindAdaptiveMomView directly so perhaps we should stick to using the Python interface there.

http://galsim-developers.github.io/GalSim/_build/html/image_class.html#galsim._Image

arunkannawadi · 2023-12-05T16:06:36Z

Eli, for this ticket, should we just move on to galsim._Image and perhaps galsim._Bounds and close this ticket? I suspect it'll be a while before a more supported API for FindAdaptiveMom will make its way to rubin-env anyway.

erykoff · 2023-12-05T16:19:08Z

@arunkannawadi This is ready for re-review. I have done what you suggest, and I didn't know about the _BoundsI constructor until now.

arunkannawadi · 2023-12-05T16:43:46Z

That was also a suggestion from Mike on Slack (DM). Per my calculations, it should save us some 5% of the total run time on single-visit images (and lesser on coadds).

arunkannawadi · 2023-12-06T22:00:01Z

python/lsst/meas/extensions/shapeHSM/_hsm_moments.py

-                hsmparams=None,
+
+            # Use galsim c++/python interface directly.
+            shape = galsim.hsm.ShapeData()


Pass in arguments here: observed_shape=galsim._Shear(), psf_shape=galsim._Shear(), moments_centroid=galsim._PositionD(). These avoid initializing objects underneath without the overhead.

arunkannawadi · 2023-12-06T22:00:21Z

python/lsst/meas/extensions/shapeHSM/_hsm_moments.py

@@ -130,21 +123,25 @@ def _calculate(
        """
        # Convert centroid to GalSim's PositionD type.
        guessCentroid = galsim.PositionD(centroid.x, centroid.y)


Make this _PositionD

arunkannawadi · 2023-12-06T22:05:15Z

python/lsst/meas/extensions/shapeHSM/_hsm_moments.py

+            shape = galsim.hsm.ShapeData()
+            hsmparams = galsim.hsm.HSMParams.default
+
+            galsim._galsim.FindAdaptiveMomView(


Right now, this is the only sticking point I believe. Either we use the FindAdaptiveMom call or have a TODO comment here referring to a ticket that should replace it with FindAdaptiveMom as soon as we have a version of GalSim with the above issue implemented.

arunkannawadi · 2023-12-06T22:44:06Z

python/lsst/meas/extensions/shapeHSM/_hsm_moments.py

-                e1=shape.observed_shape.e1,
-                e2=shape.observed_shape.e2,
+                e1=observed_shape.e1,
+                e2=observed_shape.e2,


Capturing the observed_shape in a variable is great, because getting this attribute is expensive. It constructs a Shear object, with overheads, each time observed_shape is accessed from a ShapeData object.

Use galsim primitives vs higher level interface.

ddf09c7

arunkannawadi reviewed Dec 4, 2023

View reviewed changes

python/lsst/meas/extensions/shapeHSM/_hsm_moments.py Show resolved Hide resolved

python/lsst/meas/extensions/shapeHSM/_hsm_moments.py Show resolved Hide resolved

rmjarvis reviewed Dec 5, 2023

View reviewed changes

Update to use official galsim._Image API.

c2874cf

erykoff added 2 commits December 5, 2023 08:17

Change to direct bounds constructor.

ec405c5

Add test for 100% masked image.

8843d03

erykoff force-pushed the tickets/DM-41994 branch from a69b9d3 to 8843d03 Compare December 5, 2023 16:17

arunkannawadi mentioned this pull request Dec 6, 2023

DM-41489: Reimplement C++ HSM shear measurement algorithms in Python #47

Merged

arunkannawadi approved these changes Dec 6, 2023

View reviewed changes

erykoff force-pushed the tickets/DM-41994 branch from 7356f8f to 256cea0 Compare December 6, 2023 22:40

arunkannawadi reviewed Dec 6, 2023

View reviewed changes

Add more optimized interfaces.

fe7a5ef

erykoff force-pushed the tickets/DM-41994 branch from 256cea0 to fe7a5ef Compare December 6, 2023 23:25

erykoff merged commit 87969a0 into main Dec 7, 2023
2 checks passed

erykoff deleted the tickets/DM-41994 branch December 7, 2023 05:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-41994: Use galsim primitives vs higher level interface. #48

DM-41994: Use galsim primitives vs higher level interface. #48

erykoff commented Dec 4, 2023

rmjarvis left a comment

rmjarvis Dec 5, 2023

erykoff Dec 5, 2023

rmjarvis Dec 5, 2023

erykoff Dec 5, 2023

arunkannawadi Dec 5, 2023 •

edited

rmjarvis Dec 5, 2023

arunkannawadi Dec 5, 2023

arunkannawadi Dec 5, 2023

arunkannawadi Dec 6, 2023

rmjarvis Dec 5, 2023

rmjarvis Dec 5, 2023

arunkannawadi commented Dec 5, 2023

arunkannawadi commented Dec 5, 2023

erykoff commented Dec 5, 2023

arunkannawadi commented Dec 5, 2023

arunkannawadi Dec 6, 2023

arunkannawadi Dec 6, 2023

arunkannawadi Dec 6, 2023

arunkannawadi Dec 6, 2023

DM-41994: Use galsim primitives vs higher level interface. #48

DM-41994: Use galsim primitives vs higher level interface. #48

Conversation

erykoff commented Dec 4, 2023

rmjarvis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arunkannawadi Dec 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arunkannawadi commented Dec 5, 2023

arunkannawadi commented Dec 5, 2023

erykoff commented Dec 5, 2023

arunkannawadi commented Dec 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arunkannawadi Dec 5, 2023 •

edited