Refactor image/stamp workflow #424

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.

… be made if the full image has an even number of pixels in x and/or y, then apply this offset to photon arrays before merging.

…age.

…from stamp.py.

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

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

…he functions there its static methods.

…w type.

…beginning. Previously missed. Ensure conservation of photon object fluxes during batching by adding the modulo to random batches.

…ST_ImageBuilderBase.

…than the (0,0) position.

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.

…faint objects.

…ther photon pooling tests.

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


    The transform takes 2 vectors of shape (n,) and returns
    a column major vector of shape (n,3).
    """

-    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.

Co-authored-by: Mike Jarvis <[email protected]>

…in build_obj().

…oton stamps.

…thods in photon_ops.py.

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.

… and match the new results.

…ion.

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.

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

g-braeunlich and others added 14 commits June 5, 2024 14:09

Refactor image/stamp workflow

62e8a7b

Add more doc for the photon pooling code

85cb394

Log photon objects and fix faint object flux

4920810

Fix test_image.py photon object list

e386d09

Saner batch sizes and photon pooling checkpoints to separate dataset

84115b2

Move photon pooling functions into photon_pooling.py and add docstrings.

8ead410

Automatically add Shift photon operator for LSST_Image runs.

b93f3d1

Determine each photon object's sub-pixel offset and the adjustment to…

d789e81

… be made if the full image has an even number of pixels in x and/or y, then apply this offset to photon arrays before merging.

Use copyFrom() rather than assignAt() to merge photon arrays.

d474e34

Ensure Shift photon op is used automatically with RubinOptics.

17de2ad

Fix rebase and separate tests for LSST_Image and LSST_PhotonPoolingIm…

ba11bcb

…age.

Correct use of nbatch_per_checkpoint for LSST_Image.

0413770

Re-add Shift operator manually and correct photon shooting decision.

d481c01

Correct auto-insertion of Shift photon operator and remove build_gal …

b786cb0

…from stamp.py.

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

Merge branch 'main' into u/g-braeunlich/refactor-image

0948e3c

welucas2 added 6 commits November 20, 2024 16:54

Move LSST_PhotonPoolingImageBuilder into photon_pooling.py and make t…

d897327

…he functions there its static methods.

Use simple seq of DeltaFunctions in test_image config, rather than ne…

9f1e73b

…w type.

Rename StellarObject in comments and stellar_obj variable

430240b

make_batches adds remainder objects to early batches starting at the …

c3b4737

…beginning. Previously missed. Ensure conservation of photon object fluxes during batching by adding the modulo to random batches.

Add partitioning and batching tests for photon pooling.

268e1f7

Make create_full_image and set_config_image_pos private members of LS…

eeb67dc

…ST_ImageBuilderBase.

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

PhotonPooling creates a local WCS with full_image.true_center rather …

d1fe74d

…than the (0,0) position.

welucas2 added 2 commits November 22, 2024 17:02

Batch photon objects with lower flux than nbatch as though they were …

8f0d002

…faint objects.

Add test for partitioning of low flux photon objects and strengthen o…

6bc019f

…ther photon pooling tests.

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

rmjarvis requested changes Dec 4, 2024

View reviewed changes

welucas2 and others added 10 commits December 5, 2024 07:18

Update LSST_ImageBuilder.buildImage docstring.

98bc06c

Co-authored-by: Mike Jarvis <[email protected]>

Fix docstrings, rename offset_adjustment to offset, check stamp.type …

575fc60

…in build_obj().

Rename ObjectCache to ObjectInfo.

ddefdec

Simplify __main__in test_image.py.

ed242c3

Return empty photon array with image in LSST_Photons if drawing nothing.

73197a0

Offset photons using just stamp_center when drawing the stamps.

b439a92

Nicer way of batching photon objects and conserving total flux.

a6232c6

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

c672108

…oton stamps.

Remove unused local_wcs from XyToV and the various photon_velocity me…

1aeedd3

…thods in photon_ops.py.

Have RubinOptics apply shifts using stamp_center rather than image_pos.

387c4e0

welucas2 added 2 commits December 16, 2024 16:35

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

fafaead

… and match the new results.

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

bf6cb43

…ion.

welucas2 mentioned this pull request Dec 16, 2024

Correct sky_pos and local_wcs in photon op tests #484

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor image/stamp workflow #424

Refactor image/stamp workflow #424

g-braeunlich commented Sep 28, 2023

welucas2 commented Feb 22, 2024

welucas2 commented Apr 29, 2024

welucas2 commented Jun 5, 2024

welucas2 commented Nov 20, 2024

welucas2 commented Nov 21, 2024

welucas2 commented Nov 22, 2024

rmjarvis left a comment

rmjarvis Dec 4, 2024

welucas2 Dec 5, 2024

welucas2 Dec 5, 2024

rmjarvis Dec 4, 2024

welucas2 Dec 5, 2024

rmjarvis Dec 5, 2024

rmjarvis Dec 4, 2024

welucas2 Dec 5, 2024

rmjarvis Dec 4, 2024

welucas2 Dec 5, 2024

rmjarvis Dec 4, 2024

welucas2 Dec 5, 2024

rmjarvis Dec 4, 2024

welucas2 Dec 5, 2024

rmjarvis Dec 4, 2024

welucas2 Dec 5, 2024

rmjarvis Dec 5, 2024

welucas2 Dec 6, 2024

rmjarvis Dec 6, 2024

rmjarvis Dec 4, 2024

rmjarvis Dec 4, 2024

welucas2 Dec 5, 2024

rmjarvis Dec 5, 2024

welucas2 Dec 5, 2024

welucas2 commented Dec 6, 2024

welucas2 commented Dec 16, 2024

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

Refactor image/stamp workflow #424

Are you sure you want to change the base?

Refactor image/stamp workflow #424

Conversation

g-braeunlich commented Sep 28, 2023

welucas2 commented Feb 22, 2024

welucas2 commented Apr 29, 2024

welucas2 commented Jun 5, 2024

welucas2 commented Nov 20, 2024

welucas2 commented Nov 21, 2024

welucas2 commented Nov 22, 2024

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

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

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

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

welucas2 commented Dec 6, 2024

welucas2 commented Dec 16, 2024