Skip to content
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

Save photons from the component objects of a ChromaticSum #1285

Closed

Conversation

welucas2
Copy link

This commit addresses #1284 by checking whether image already has a photons attribute at the end of GSObject.drawImage. If not, it simply assigns it the newly created photon array as previously. If it does exist, a new array with the correct length is created, the extant image.photons copied in at the beginning and the new photons after, and the new array assigned to image.photons.

This fix corrects the example in #1284 and also fixes previously incorrect output from the imSim photon pooling pipeline noted in LSSTDESC/imSim#424.

@rmjarvis
Copy link
Member

rmjarvis commented May 1, 2024

@welucas2 You'll need to rebase this to the current main to get the fix to CI in #1288.

…y in an image returned by GSObject.drawImage rather than replacing with new array.
@welucas2 welucas2 force-pushed the chromaticsum_photons branch from 34e138c to 2f65ad7 Compare May 1, 2024 17:12
@welucas2
Copy link
Author

welucas2 commented May 1, 2024

Thanks for the heads-up @rmjarvis, just rebased.

@rmjarvis
Copy link
Member

rmjarvis commented May 1, 2024

I don't think this is the right fix for this. If you repeatedly draw things and save photons each time, we won't want more photons appended each time. We'd want them overwritten. I think the fix needs to happen in ChromaticSum itself, not in GSObject. But rather than have you delve into the guts of the code, I can tackle this along with #1271.

@welucas2
Copy link
Author

welucas2 commented May 1, 2024

I'm certainly not familiar with all the use cases so I'll take that as the correct approach! I guess the imSim photon pooling is only concerned that the photon array coming back out again from ChromaticSum contains the complete set of photons drawn for the object as a whole. If doing it in ChromaticSum, maybe the full-sized array can be created and then photons coming back from drawImage can be copied in analogously to added_flux..?

@rmjarvis
Copy link
Member

rmjarvis commented May 6, 2024

See #1289 instead.

@rmjarvis rmjarvis closed this May 6, 2024
@rmjarvis rmjarvis added this to the No action milestone May 6, 2024
@rmjarvis rmjarvis added bug report Bug report chromatic Related to the Chromatic classes, SEDs, bandpasses, etc. labels May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug report Bug report chromatic Related to the Chromatic classes, SEDs, bandpasses, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants