search

GalSim-developers / GalSim

The modular galaxy image simulation toolkit. Documentation:
http://galsim-developers.github.io/GalSim/
Other
227 stars 107 forks source link

#1156 Fix error in ChromaticSum photon shooting when n_photons is given explicitly #1157

Closed rmjarvis closed 2 years ago

rmjarvis commented 2 years ago

See #1156 for description of the problem.

This PR fixes it and adds tests to confirm.

jmeyers314 commented 2 years ago

Looks good to me.

If there's an easy way to aggregate the photons from the different summands before applying the extra photon_ops, I think that'd be a win (batoid traces 40,000 photons in 1 batch faster than 20000+20000 photons split into two batches). But I'd understand if you want to leave that to a future issue rather than as part of this bugfix.

cwwalter commented 2 years ago

Curious why you took out the f-strings. Is there a speed issue?

rmjarvis commented 2 years ago

They don't work on python 3.7, which we still support. And it's in the tests, so speed isn't relevant.

cwwalter commented 2 years ago

Ah. 3.7. OK thanks.

rmjarvis commented 2 years ago

If there's an easy way to aggregate the photons from the different summands before applying the extra photon_ops, I think that'd be a win.

I think a lot of the chromatic photon shooting could be usefully refactored in a way that would enable this. But right now I think it would be difficult to implement that.