GalSim-developers / GalSim

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

Implement ChromaticSum._shoot #1259

Closed rmjarvis closed 10 months ago

rmjarvis commented 10 months ago

In PR #1100, I implemented the applyTo method for chromatic objects, so they can be used as photon_ops. This is now the canonical way to do photon shooting with chromatic objects, since it can use each photon's specific wavelength to do wavelength-dependent things.

One that I skipped in that PR was ChromaticSum. I suspect I thought that class would only be used for things like bulge + disk, where each object has a spectral SED. These can't be used as photon_ops, so I didn't implement that functionality for them.

However, @matroxel wants to use ChromaticSum for the RomanPSF to smoothly interpolate across a sensor using a weighted sum of 4 PSFs, corresponding to the 4 corners of the sensor. So this PR rectifies my oversight, and implements ChromaticSum._shoot, which is the back end function for all PSF applyTo calls.

Note: when implementing this, I added PhotonArray._copyFrom, which can copy portions of the data from one PhotonArray to another. It copies all the allocated arrays, using an arbitrary mask in both the original and target PhotonArray. Similar to assignAt but more flexible. (And assignAt now uses _copyFrom for its implementation.)

I now use this all the places where we were doing this. I think this is safer than what we were doing, since some PSFs may need things like the pupil positions or time and when generating the positions/fluxes, they may have generated such things. So it's a little bit less efficient in most cases (but not much) in the interest of avoiding potentially subtle bugs that may turn up in various edge cases. (We don't currently have any unit tests that care about such things.)

rmjarvis commented 10 months ago

Question for you. Do you think we should deprecate assignAt as well in favor of the new copyFrom?
I think the new name is clearer (and matches a similar method in Image). The only place we had used assignAt was in the old Sum._shoot method, so that's now gone with the new algorithm. And I doubt any users probably use assignAt for anything.

jmeyers314 commented 10 months ago

Do you think we should deprecate assignAt as well in favor of the new copyFrom?

Yep, this sounds good to me.