Refactor image/stamp workflow by g-braeunlich · Pull Request #424 · LSSTDESC/imSim

g-braeunlich · 2023-09-28T12:13:48Z

No description provided.

welucas2 · 2024-02-22T13:55:55Z

The latest commit aligns photon objects in the photon pooling pipeline with their positions in the original pipeline.

welucas2 · 2024-04-29T14:43:27Z

Almost there, but blocked by GalSim-developers/GalSim#1284.

welucas2 · 2024-06-05T13:35:15Z

This is finally open for review -- all comments welcome.

rmjarvis

Thanks William. I have a bunch of comments, but this is definitely a good start. If it would be helpful to talk through any of this on a zoom call, let me know.

rmjarvis · 2024-08-14T17:47:44Z

config/imsim-config.yaml

+    # If using photon pooling, FFT and photon objects are batched separately.
+    # There will probably be few enough FFT objects to do in a single batch.
+    # Bright photon objects are treated in all photon batches, but at 1/nbatch_photon of
+    # their flux. Faint photonobjects are placed at their full flux in random batches.


Suggested change

# their flux. Faint photonobjects are placed at their full flux in random batches.

# their flux. Faint photon objects are placed at their full flux in random batches.

Thanks for spotting this. Fixing in the next commit.

rmjarvis · 2024-08-14T17:49:56Z

config/imsim-config.yaml

+    # These parameters only affect LSST_PhotonPoolingImage images.
+    # The batch numbers can be changed if desired.
+    nbatch_fft: 1
+    nbatch_photon: 10


I think this should be the same parameter as the simple nbatch. We can just document that with photon pooling, nbatch refers only to the photon-shooting objects. Then nbatch_fft can be an optional parameter that defaults to 1. (Maybe not even worth mentioning here if most users wouldn't change it?)

I agree with this. Nice to keep the number of parameters down. nbatch_photon will be no more, and the photon pooling will be controlled directly with nbatch.

rmjarvis · 2024-08-14T17:53:58Z

imsim/lsst_image.py

+
+class LSST_ImageBuilder(LSST_ImageBuilderBase):
+    """This is mostly the same as the GalSim "Scattered" image type.
+    So far the only change is in the sky background image."""


This doc is probably obsolete. There are quite a few differences now.

Happy to change this. Can you suggest something more suitable?

rmjarvis · 2024-08-14T17:55:18Z

imsim/lsst_image.py

-                'x' : { 'type' : 'Random' , 'min' : xmin , 'max' : xmax },
-                'y' : { 'type' : 'Random' , 'min' : ymin , 'max' : ymax }
-            }
+        set_config_image_pos(config, base)


Rather than making this a free function in a different file, this should probably be a private method of the base class, which both subclasses can call.

Agreed, doing this.

rmjarvis · 2024-08-14T18:01:06Z

imsim/lsst_image.py

+            base["stamp"]["photon_ops"] = []
+        photon_ops = base["stamp"]["photon_ops"]
+        shift_op = {'type': 'Shift'}
+        shift_index = next((index for (index, d) in enumerate(photon_ops) if d["type"] == "Shift"), None)


I don't love this implementation. I get why something like this is now required. But I think I'd rather have RubinOptics and RubinDiffractionOptics just have an optional parameter to apply this shift or not. Then when running this class, base[image_pos] is defined and they can apply the shift based on that.

In the photon pooling class, once we've pooled all the photons, we should set the object-specific values (image_pos, sky_pos, maybe others) that apply to a particular object to None. Then the RubinOptics and RubinDiffractionOptics photon ops would see that the image_pos isn't defined, so not apply any shift.

This feels to me more elegant and possibly more efficient.

This should now be done -- see RubinOptics and RubinDIffractionOptics. If an optional shift_photons = True is passed, the shift is performed.

rmjarvis · 2024-08-16T21:24:16Z

tests/test_image.py

+        },
+        "psf": {"type": "Convolve", "items": [{"type": "Gaussian", "fwhm": 0.3}]},
+        "stamp": {
+            "type": "LSST_Silicon",


This should be the new stamp type when image_type is LSST_PhotonPoolingImage

Yes, photon pooling now needs to use the new stamp type rather messing with base. The test sets the correct stamp type.

rmjarvis · 2024-08-16T21:29:59Z

tests/test_image.py

+
+def test_lsst_image_photon_pooling_pipeline():
+    """Check that LSST_PhotonPoolingImage batches objects as expected and renders objects at the correct positions."""
+    run_lsst_image("LSST_PhotonPoolingImage")


I feel like we probably want a few additional tests in this pipeline. E.g. that the FFT and faint objects only got drawn once. And the others got drawn nbatch times.

I've written a set of tests for the partitioning and batching process. As mentioned above, it also confirms that total flux is conserved for photon objects,

rmjarvis · 2024-08-16T21:33:36Z

tests/test_image.py

+        help="Similar to -k of pytest / unittest: restrict tests to tests starting with the specified prefix.",
+        default="test_",
+    )
+    args = parser.parse_args()


FWIW, when working on stuff, I usually just put the name of the test function I'm working on at the start of this name == '__main__' block, followed by quit(). Then remove it before committing. Seems easier than rolling all of this argparse stuff, but 🤷.

This is also done in test_diffraction_fft.py, but I'm happy to change it if you'd like me to.

I guess if you find it useful, go ahead and keep it. But if you were just copying from that file, I don't think this is necessary to keep.

rmjarvis · 2024-08-16T21:37:42Z

tests/test_photon_ops.py

-    expected_x_pic_center = 564.5
-    expected_y_pic_center = -1431.4
+    expected_x_pic_center = -989.5971378245167
+    expected_y_pic_center = -3840.3512012842157


Why are these (and subsequent tests) changing so much? This isn't close.

Regression tests are supposed to make sure we don't change the functionality of existing code. I know the implementation of these photon ops changed, but I think we want the tests to remain the same or at least very very close. What happened here?

This is still a big issue. I'm 99% convinced the error is in the original test itself. My current best guess is that with the old code it may not have been internally self-consistent around the WCS used, while the new XyToV should enforce self-consistency. But it still needs to be looked at.

rmjarvis · 2024-08-16T21:39:14Z

tests/test_stamp.py

        "bandpass": galsim.Bandpass('LSST_r.dat', wave_type='nm'),
-        "wcs": galsim.PixelScale(0.2),
+        "wcs": wcs,
+        "current_image": galsim.Image(1024, 1024, wcs=wcs)


Why was this change required?

The change to the wcs field isn't actually necessary, it looks. The addition of current_image is needed for deserialize_rubin_optics, deserialize_rubin_diffraction_optics and deserialize_rubin_diffraction which uses it to get img_wcs, required by the new version of XyToV.

welucas2 · 2024-11-07T13:44:11Z

The latest set of commits should address most of the comments above. I'll respond to those individually. A couple do still need to be looked at.

rmjarvis · 2024-11-19T19:46:04Z

Are you waiting on me for anything here? I thought you still had some more comments to address, so I was waiting for that to be done. But if it would be helpful for me to review this again at this point, let me know.

welucas2 · 2024-11-20T16:00:17Z

Not just yet -- I have a couple more commits on the way with fixes and tests for the batching. I'll push these later today or tomorrow, and then I think that should be everything ready for review again.

welucas2 · 2024-11-20T17:59:32Z

I think this is ready for re-review now, though CI is failing with an error in test_stamp_bandpass_airmass from numpy coming through GalSim. I saw the same error a couple of weeks ago on an ARM system when using GalSim 2.6 which introduces its own utilities.least_squares(). I wasn't able to investigate further before the system went down and only came back up yesterday.

welucas2 · 2024-11-21T18:52:42Z

I think there is actually one more commit incoming. I've realised that in edge cases where nominally bright photon objects' phot_flux falls below nbatch, we can end up with batches containing objects with 0 flux. I think I have a fix, by having these objects be treated as faint objects for batching purposes. I'll test this and if all is well push it tomorrow.

welucas2 · 2024-11-22T17:05:29Z

OK, that should be it -- code open for review!

rmjarvis

Thanks William. I think we're close.

I'd still like to have a separate PR that just changes the tests of the expected centers. I think the way to do that is to change the test to mostly your new code, but when calling (the old) XyToV, use sky_pos = img_wcs.toWorld(image_pos) and local_wcs = img_wcs.local(image_pos). I'm not sure what image_pos is exactly, but hopefully you can follow Geri's code to see what the implicit image_pos is there.

imsim/lsst_image.py

rmjarvis · 2024-12-04T18:52:01Z

imsim/photon_pooling.py

+            offset_adjustment: a PositionD by which to offset each object
+        """
+        for stamp in stamps:
+            offset = offset_adjustment


Probably should just call this parameter offset from the start.

Thanks! Doing this now.

And now obsolete with the big simplification in offsetting the photons by stamp_center. I've removed the two static methods in photon_pooling.py dealing with the offset and it's now dealt with in stamp.py just using stamp_center.

rmjarvis · 2024-12-04T18:57:30Z

imsim/photon_pooling.py

+        Returns:
+            images: Tuple of the output stamps. In normal PhotonPooling usage these will actually be
+                PhotonArrays to be processed into images after they have been pooled following this step.
+            current_vars: Tuple of variables for each stamp (noise etc).


These should be the current noise variance in each image. var=variance, not variable.
It's not really relevant for most imsim usage, but if you draw a RealGalaxy, and especially if you then whiten it, the image gets some noise from that, so this mechanism helps GalSim from adding more noise than requested when adding the rest of the image noise.

Thanks for pointing this out. Is the actual usage of current_vars ok?

Yes. Just the doc string is wrong.

rmjarvis · 2024-12-04T19:08:48Z

imsim/photon_pooling.py

+        batches = [
+            [dataclasses.replace(obj, phot_flux=np.floor(obj.phot_flux / nbatch)) for obj in phot_objects]
+        for _ in range(nbatch)]


A nifty (simpler?) way to get the total flux to come out right:

Suggested change

batches = [

[dataclasses.replace(obj, phot_flux=np.floor(obj.phot_flux / nbatch)) for obj in phot_objects]

for _ in range(nbatch)]

batches = [

[dataclasses.replace(obj,

phot_flux=(obj.phot_flux*(i+1))//nbatch - (obj.phot_flux*i)//nbatch)

) for obj in phot_objects]

for i in range(nbatch)]

Then you don't need the block below this.

Very nice -- thanks! Putting this in now.

rmjarvis · 2024-12-04T19:17:07Z

imsim/stamp.py

+def build_obj(stamp_config, base, logger):
+    """Precompute all data needed to determine the rendering mode of an
+    object."""
+    stamp_type = stamp_config.get('type','LSST_Photons')


I don't think we want to have a default here. Rather we should make sure the user has correctly specified this type. So if stamp_config.get('type', None) != 'LSST_Photons':... then raise some kind of explanatory errror.

Thanks -- doing this now.

rmjarvis · 2024-12-04T20:00:35Z

imsim/stamp.py

+
+        max_flux_simple = config.get('max_flux_simple', 100)
+        faint = self.nominal_flux < max_flux_simple
+        bandpass = base['bandpass']


D'oh! Thanks for spotting -- fixed.

rmjarvis · 2024-12-04T20:25:15Z

imsim/stamp.py

+            img_pos = base["image_pos"]
+            obj_offset = base["stamp_offset"]
+            image.photons.x = image.photons.x + img_pos.x - obj_offset.x
+            image.photons.y = image.photons.y + img_pos.y - obj_offset.y


I think all this offset stuff could be simpler. Here you add img_pois - stamp_offset. Then later you add 0.5 for even sized images. So the net adjustment is

img_pos - stamp_offset + (0.5 if size%2==0)

I'm pretty sure this combination is equivalent to base["stamp_center"] based on the calculation of stamp_offset here. So I think you can just add stamp_center here and skip the later adjustments. Which honestly makes sense. These photon arrays are meant to be relative to the image center, which here is just the stamp center.

Very nice call. I've just tested and this gives identical output. Making this change now.

rmjarvis · 2024-12-04T21:39:25Z

imsim/photon_ops.py

+        # positions the photons before they are pooled to be processed together.
+        if self.shift_photons and self.image_pos is not None:
+            photon_array.x += self.image_pos.x
+            photon_array.y += self.image_pos.y


Per my other comment, these should presumably also shift by stamp_center, not image_pos. Likely not a big difference, but might as well use the correct shift.

Should this also be done at the end of applyTo? e.g.

imSim/imsim/photon_ops.py

Lines 111 to 112 in a4c2d97

photon_array.x -= self.image_pos.x

photon_array.y -= self.image_pos.y

in main as it is now.

Yes. I think so.

This is the last thing on my list. It means changing the API to RubinOptics and RubinDiffractionOptics. I think normal use is always through photon op registrations via the photon_op_type decorator and deserialize_* functions, which wouldn't need to change, but at least the tests in test_photon_ops.py do directly init the photon operators themselves. Is that ok to change?

I think if we preserve the user-facing code (i.e. how the config file specifies things), then it's ok to make changes to the implementation, even if these are technically API changes. But perhaps @jchiang87 or @cwwalter might want to weigh in about how strictly we want to preserve backwards-compatibility of the Python classes/functions.

rmjarvis · 2024-12-04T22:12:55Z

tests/test_image.py

+        help="Similar to -k of pytest / unittest: restrict tests to tests starting with the specified prefix.",
+        default="test_",
+    )
+    args = parser.parse_args()


I guess if you find it useful, go ahead and keep it. But if you were just copying from that file, I don't think this is necessary to keep.

rmjarvis · 2024-12-04T22:23:33Z

imsim/photon_ops.py

    """

-    def __init__(self, local_wcs, icrf_to_field, sky_pos):
+    def __init__(self, local_wcs, icrf_to_field, img_wcs):


I think this doesn't need local_wcs anymore. If we're breaking the API (with the change in the last parameter), might as well clean it up and remove local_wcs as well.

Done. This also leaves some of the photon_velocity methods in RubinOptics, RubinDiffraction and RubinDiffractionOptics with unnecessary local_wcs and, now I look at them, also rng. Would you prefer those left alone or removed as well?

Might as well clean up all the obsolete API. If any of them seem like user-facing code, we should make this a deprecation rather than completely break old code. But I think most of these functions are internal, not user facing, so it should be ok.

I forgot to mention this in the meeting! Yes, everything looks to be internal to the photon ops themselves. rng can't be removed though, as it turns out, or it breaks RubinDiffractionOptics. Otherwise, tidied up as requested.

welucas2 · 2024-12-06T18:05:10Z

Assuming tests pass, that should be everything above. I've included changes to RubinOptics to use stamp_center rather than image_pos, changing the tests to fit, but let me know if you don't like that and I'll revert it.

I'll move on to seeing if I can make the old photon ops tests match the new ones as suggested.

welucas2 · 2024-12-16T17:07:02Z

I've just created PR #484 which corrects the photon op tests to be self-consistent. There were actually a couple of inconsistencies in the tests here too, but working through them now means everything should be in alignment pre- and post-photon pooling.

I hope this means that once #484 is merged in, this can be, too.

jchiang87

Hi William,

In comparing how this branch works wrt the code in main, I saw a few items that may need to be addressed.

jchiang87 · 2025-01-06T20:01:41Z

config/imsim-config.yaml

+    # If using LSST_Image (non-photon pooling), nbatch is the overal number of batches in
+    # which to draw the objects.
+    # The default number of batches is 100, but you can change it if desired.
+    nbatch: 100


It would be good to clarify that nbatch is also used by the photon pooling code, not just by LSST_Image.

Thanks -- doing this now.

jchiang87 · 2025-01-06T20:02:53Z

imsim/photon_pooling.py

+        imview._shift(-center)  # equiv. to setCenter(), but faster
+        imview.wcs = PixelScale(1.0)
+        if imview.dtype in (np.float32, np.float64):
+            sensor.accumulate(photons, imview, imview.center)


Is there ever a situation where we would want to set resume=True here, e.g., similar to this case in galsim?

I'm not sure -- it looks like it can boost performance somewhat when using a SiliconSensor by skipping an initial calculation in the accumulate method. I don't think there's any disadvantage to using resume=True, but it might be worthwhile discussing this further in tomorrow's call to be certain.

Making this change now.

Actually, though the tests pass it looks like my implementation has problems in practice as it's a different image view being used each time. I'm trying a fix now by creating the image view outside the loop over photon pools and reusing it.

Should be fixed with the latest commit.

jchiang87 · 2025-01-06T20:28:17Z

imsim/stamp.py

+                        image=image,
+                        wcs=base['wcs'],
+                        sensor=NullSensor(), # Prevent premature photon accumulation
+                        photon_ops=psfs,


Should this be

photon_ops=photon_ops,

?

You're right, it should be photon_ops, but the code above which creates the initial photon_ops list from the config is incorrect -- thanks for noticing something's not right here. It should be contain the PSF and the BandpassRatio photon op (only if the SED needs to be reweighted), which need to be dealt with here. All the other photon ops are dealt with later in the pipeline and applied to the whole photon batch. I'm fixing this now.

jchiang87 · 2025-01-06T20:33:21Z

imsim/stamp.py

+                        save_photons=True)
+            stamp_center = base['stamp_center']
+            image.photons.x = image.photons.x + stamp_center.x
+            image.photons.y = image.photons.y + stamp_center.y


The non-photon pooling code sets

base['realized_flux'] = image.added_flux

at the corresponding point in the code so that those values appear in the output.truth files. What should it be set to here so that the equivalent info is written out?

This is a tricky one. Per object quantities are diluted with the use of photon pools. If we did the same thing here in the stamp, we'd firstly only be dealing with an object flux which is a fraction ~1/nbatch of the total flux, complicated by different pools having different integer values of the flux in order to conserve the total. Furthermore it might have been possible to track and sum the realised flux per object across the pools, but the accumulation now happens at the pool level. We can tell how much of the pool's flux is added to the image, but the information about which object contributed which photon is not retained at that point. I'm not sure if someone else has a suggestion or is aware of something that would help. I'll have a think about this but I think we might need to chat about this in tomorrow's call.

…oton stamps.

…thods in photon_ops.py.

… and match the new results.

…ion.

…r performance with SiliconSensors.

…ton objects across pools for an incident_flux column.

…ormance in photon accumulation.

…help the garbage collector prevent OOM.

welucas2 · 2025-05-09T12:38:41Z

Rebased onto #485's fix to the CI, tests are now passing. @rmjarvis, @jchiang87, are you happy to merge?

rmjarvis · 2025-05-09T18:50:32Z

imsim/lsst_image.py

+        # nbatch is used for LSST_Image and LSST_PhotonPooling runs. In the former it determines
+        # the number of batches into which to all objects are placed, while in the latter it is
+        # used only with photon objects (bright and faint).
+        self.nbatch = params.get('nbatch', 100)


If we're doing subbatches, I think 100 is probably overkill for the main batch number. 10 or 20 should be sufficient for the science needs of getting BFE correct. Maybe adjust the distribution of these two. (E.g. default nbatch=20, nsubbatch=50 to keep 1000 total.)

Also (related) nsubbatch isn't included in the config file, which kind of serves as our documentation of this stuff. Probably worth adding there.

What about for LSST_Image runs -- are we happy changing the default for people who will continue using that process? I could have it use a different value depending on image type, 100 for LSST_Image, 10 or 20 for LSST_PhotonPoolingImage.

I set nsubbatch in the new photon pooling config (imsim-config-photon-pooling.yaml) since it isn't used for LSST_Image runs. I can move it to imsim-config.yaml if you'd like, or document it where it is now.

Our template config file for LSST_Image sets nbatch explcitly, so maybe leave it 100 there, but change the comment to say that the default is 20, and that there is a speed/memory tradeoff involved, so when memory is at a premium, increasing it to something like 100 is generally warranted.

Thanks. I've switched things around to move nsubbatch from imsim-config-photon-pooling.yaml into imsim-config.yaml, and added some docs, and used your suggested defaults of nbatch=20 and nsubbatch=50 for photon pooling (and keeping nbatch=100 for LSST_Image). I might push a little for a default of nbatch=10 for photon pooling with which runs take about 60% the time of nbatch=20, but at least it's something that can be easily tuned.

I'm fine with a default of 10, rather than 20. I think almost all use cases will not care about the difference between these. 20 is more conservative, but I doubt 10 will ever be insufficient.

I've changed again to 10 and noted this in the config comment.

rmjarvis · 2025-05-09T19:26:12Z

imsim/photon_pooling.py

+                    op.applyTo(photons, local_wcs, rng)
+                # Shift photon positions to be relative to full_image.center.
+                # This is necessary as all photons will now be in the pool together, and there
+                # is no longer any way to distinguish which belong to which object.


It's more that this is necessary because the accumulate function assumes x,y are relative to the center of the image, which for this process is the full image, not the stamp.

In fact, I think this is probably more complicated than necessary. You have a few things here that enforce this, which we could probably get rid of rather than do the following two lines.
The accumulate function has an option orig_center, which defaults to (0,0), but where you use imview.center. I think it's probably cleaner to not use that parameter at all and not shift the photon x,y by the center position. Then the accumulate function would use the normal x,y positions on the full image's normal grid.
So I recommend the following changes:

Remove the next two lines and the above comment about them.

Remove the orig_center option to accumulate below (given as an unnamed arg, now a kwarg -- it's your third arg).

Remove the imview._shift call above. Which ironically means that the orig_center argument isn't doing anything, since the center of the view is 0,0, which is the default.

Maybe even stop using the imview object entirely. I think that was just used in the old code because we wanted to shift the stamp centers to 0,0.

Thanks, this sounds good. I'll make these changes and check I'm not breaking anything else.

All looks good, and that's definitely made things simpler. Thanks for spotting it! Making the change now.

rmjarvis · 2025-05-09T19:38:35Z

imsim/photon_pooling.py

+        # Regular Sensors don't simulate this so don't accept recalc.
+        if imview.dtype in (np.float32, np.float64):
+            if isinstance(sensor, SiliconSensor):
+                sensor.accumulate(photons, imview, imview.center, resume=resume, recalc=recalc)


I already suggested removing imview.center here. But if I hadn't, then this should really be a named kwarg, not an arg, since otherwise it's not clear from the call what exactly it means.

Thanks -- I'll go with removing imview.center as above.

And done with its removal.

rmjarvis · 2025-05-09T19:54:28Z

imsim/photon_pooling.py

+RegisterImageType('LSST_PhotonPoolingImage', LSST_PhotonPoolingImageBuilder())
+
+
+class PhotonPoolingTruthBuilder(TruthBuilder):


What is this for? It doesn't seem to be used anywhere (tests or example config files).

It seems to be basically just recapitulating the regular TruthBuilder with a special case for incident_flux. I suspect we could do this another way that doesn't require so much duplicative code.

This (or rather something with the same effect) was a request of Jim's from above and discussed in the calls in January/February. With photon pooling there's no way of getting the realized_flux for photon objects as they go into the pool before being accumulated. This was an attempt to at least get the flux in all the photons for each object into the truth file, even if it includes some that isn't actually accumulated. Better than having 0. It's currently used in imsim-config-photon-pooling.py, which also disables the regular truth.

I could maybe try doing the sum of photons across the objects during the pooling loop in the image builder. But wouldn't that then need processImage instead of processStamp?

I've got a new implementation which mostly uses the inherited code from the regular GalSim truth, only doing a little extra to sum the total flux in the photon shooting objects. I've also added a small assertion to the photon pooling test in test_image.py which checks that the nominal flux and incident flux are within 10% of one another. That could maybe be changed e.g. to within Poisson noise if you'd prefer.

rmjarvis · 2025-05-09T19:56:46Z

tests/test_photon_ops.py

    if icrf_to_field is None:
        icrf_to_field = create_test_icrf_to_field(boresight, det_name=det_name)
    img_wcs = create_test_img_wcs(boresight, rottelpos)
+    img_wcs = create_test_img_wcs(boresight, rottelpos)


Thanks - removing the extra line now.

rmjarvis · 2025-05-09T19:57:26Z

tests/test_photon_ops.py

    altitude=89.9 * degrees,
-    sky_pos=galsim.CelestialCoord(0.543 * galsim.radians, -0.174 * galsim.radians),
    icrf_to_field=None,
+    rottelpos=np.pi / 3 * galsim.radians,


Maybe more intuitive as 60 * galsim.degrees.

Thanks, doing this now throughout the photon op tests.

rmjarvis · 2025-05-09T19:59:37Z

tests/test_photon_ops.py

    icrf_to_field=None,
-    sky_pos=galsim.CelestialCoord(0.543 * galsim.radians, -0.174 * galsim.radians),
-    image_pos=galsim.PositionD(809.6510740536025, 3432.6477953336625),
+    stamp_center=galsim.PositionD(809.6510740536025, 3432.6477953336625),


This doesn't make sense as a stamp center. An image pos can be arbitrary with lots of digits, but a stamp center is always either integers or half-integers. I guess it probably doesn't matter much for this test what the value is exactly, so maybe just change it to (809.5, 3432.5) to assuage my cognitive dissonance.

Sounds good -- thanks! That definitely makes sense. Fixing this now.

rmjarvis · 2025-05-09T20:04:45Z

tests/test_photon_ops.py

    """Check the config interface to RubinDiffractionOptics."""

-    image_pos = galsim.PositionD(3076.4462608524213, 1566.4896702703757)
+    stamp_center = galsim.PositionD(3076.4462608524213, 1566.4896702703757)


ditto here and a few other places I guess.

Fixing throughout.

rmjarvis · 2025-05-09T20:10:09Z

imsim/photon_pooling.py

+            # Create a temporary ImageD to work in.
+            im1 = galsim.image.ImageD(bounds=imview.bounds)
+            if isinstance(sensor, SiliconSensor):
+                sensor.accumulate(photons, im1, imview.center, resume=resume, recalc=True)


I think recalc and resume won't work here, since it's always a new image. We probably don't ever use this branch in imsim, but just in case, maybe issue a warning if resume is True or recalc is False that these don't work when the image type is not float32 or float64.

Thanks for the suggestion -- adding warnings now.

…don't bother with the shifting back and forth.

…nd provide a bit more documentation.

…iew any more) and issue warnings if using a non-float image and trying to resume or suppress recalculation.

… less.

…n test_image.py.

welucas2 · 2025-06-05T10:24:54Z

And done! Thanks everyone.

g-braeunlich requested a review from rmjarvis September 28, 2023 12:13

g-braeunlich self-assigned this Sep 28, 2023

g-braeunlich force-pushed the u/g-braeunlich/refactor-image branch 2 times, most recently from e58012d to c145f0c Compare October 10, 2023 12:34

g-braeunlich force-pushed the u/g-braeunlich/refactor-image branch from c145f0c to 1d7fefc Compare October 30, 2023 09:16

g-braeunlich force-pushed the u/g-braeunlich/refactor-image branch 2 times, most recently from acbac83 to 9f49df7 Compare November 30, 2023 12:45

welucas2 self-assigned this Apr 29, 2024

welucas2 mentioned this pull request Apr 30, 2024

Save photons from the component objects of a ChromaticSum GalSim-developers/GalSim#1285

Closed

welucas2 force-pushed the u/g-braeunlich/refactor-image branch from daae77c to 7290a11 Compare May 24, 2024 12:11

welucas2 force-pushed the u/g-braeunlich/refactor-image branch from bdaadae to b786cb0 Compare June 5, 2024 13:10

welucas2 marked this pull request as ready for review June 5, 2024 13:32

rmjarvis requested changes Aug 16, 2024

View reviewed changes

welucas2 force-pushed the u/g-braeunlich/refactor-image branch from 46461fb to eeb67dc Compare November 20, 2024 16:54

welucas2 requested a review from rmjarvis November 22, 2024 17:05

rmjarvis requested changes Dec 4, 2024

View reviewed changes

welucas2 mentioned this pull request Dec 16, 2024

Correct sky_pos and local_wcs in photon op tests #484

Merged

jchiang87 requested changes Jan 6, 2025

View reviewed changes

welucas2 force-pushed the u/g-braeunlich/refactor-image branch from bf6cb43 to 72cc687 Compare January 17, 2025 11:54

welucas2 added 15 commits May 9, 2025 12:49

Nicer way of batching photon objects and conserving total flux.

bf07d5e

Use BandpassRatio as a photon op (if correct to do so) within LSST_Ph…

397059f

…oton stamps.

Remove unused local_wcs from XyToV and the various photon_velocity me…

e1e18bd

…thods in photon_ops.py.

Have RubinOptics apply shifts using stamp_center rather than image_pos.

27b58f1

Update most photon op tests to resemble updated self-consistent tests…

1f2c63f

… and match the new results.

Allow RubinDiffraction to shift photons and fix tests to use this opt…

660fe67

…ion.

Apply bandpass ratio and PSFs in LSST_Photons for photon objects.

b3bc184

Note nbatch parameter is also used in photon pooling.

2ffe411

Pass resume to sensor.accumulate in photon pooling for slightly bette…

86a2e14

…r performance with SiliconSensors.

Create imview at start of photon pooling and reuse.

27448a3

Photon pooling uses new photon_pooling_truth which sums flux from pho…

0d8a1d1

…ton objects across pools for an incident_flux column.

Use the SiliconSensor recalc option from GalSim 2.7 for improved perf…

5db525a

…ormance in photon accumulation.

Manually del the stamps and merged photon array in photon pooling to …

862f29a

…help the garbage collector prevent OOM.

Initial go at subbatching photon pools.

781e932

Add tests for sub-batching.

79d7eac

welucas2 force-pushed the u/g-braeunlich/refactor-image branch from f597359 to 79d7eac Compare May 9, 2025 11:50

rmjarvis requested changes May 9, 2025

View reviewed changes

welucas2 added 8 commits May 27, 2025 11:02

Remove duplicate line in create_test_rubin_optics_kwargs

3138d61

Make stamp_centers half-pixel positions throughout test_photon_ops.py.

a25d7e2

Change angle units to degrees for rottelpos in test_photon_ops.py.

8af0812

Photon pooling: accumulate to full_image without using the view, and …

c210242

…don't bother with the shifting back and forth.

Change default nbatch and nsubbatch, move to main imsim-config.yaml a…

f47fdb3

…nd provide a bit more documentation.

accumulate_photons: Change imview to image (since we aren't using a v…

3c4b92e

…iew any more) and issue warnings if using a non-float image and trying to resume or suppress recalculation.

Refactor PhotonPoolingTruthBuilder to call TruthBuilder and duplicate…

8a71d5d

… less.

Include a photon pooling truth assertion in the photon pooling test i…

743b440

…n test_image.py.

rmjarvis approved these changes May 30, 2025

View reviewed changes

jchiang87 approved these changes Jun 3, 2025

View reviewed changes

welucas2 merged commit 8d9c804 into main Jun 5, 2025
4 checks passed

	# their flux. Faint photonobjects are placed at their full flux in random batches.
	# their flux. Faint photon objects are placed at their full flux in random batches.

	photon_array.x -= self.image_pos.x
	photon_array.y -= self.image_pos.y

		RegisterImageType('LSST_PhotonPoolingImage', LSST_PhotonPoolingImageBuilder())


		class PhotonPoolingTruthBuilder(TruthBuilder):

Conversation

g-braeunlich commented Sep 28, 2023

Uh oh!

welucas2 commented Feb 22, 2024

Uh oh!

welucas2 commented Apr 29, 2024

Uh oh!

welucas2 commented Jun 5, 2024

Uh oh!

rmjarvis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

welucas2 commented Nov 7, 2024

Uh oh!

rmjarvis commented Nov 19, 2024

Uh oh!

welucas2 commented Nov 20, 2024

Uh oh!

welucas2 commented Nov 20, 2024

Uh oh!

welucas2 commented Nov 21, 2024

Uh oh!

welucas2 commented Nov 22, 2024

Uh oh!

rmjarvis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!