Implement ChromaticSum._shoot #1259
Conversation
rmjarvis
added
roman
…sed in ChromaticSum
…otons, which is now unnecessary
| ret._is_corr = False | ||
| if is_corr: | ||
| from .deprecated import depr | ||
| depr('is_corr=True', 2.5, '', |
There was a problem hiding this comment.
What was the former use case and why is it no longer necessary?
There was a problem hiding this comment.
It was for the Sum._shoot implementation. That had done all the photons in one component first, then all the ones in the other component. I switched that to do the same algorithm I used for ChromaticSum. This is potentially important if the PSF is time dependent for instance (e.g. includes a PhasePSF), so the order in the PhotonArray matters. It seems like there are subtle implications to letting them be correlated like that, and it was very easy to switch it to not ever be correlated in the first place.
| assert_raises(ValueError, pa2.copyFrom, pa1, 100, 0) | ||
| assert_raises(ValueError, pa2.copyFrom, pa1, 0, slice(None)) | ||
| assert_raises(ValueError, pa2.copyFrom, pa1) | ||
|
|
There was a problem hiding this comment.
Should probably add some tests for numpy fancy indexing too.
I.e., pa1.copyFrom(pa2, pa2.flux>10) and pa1.copyFrom(pa2, np.where(pa2.flux>10))
There was a problem hiding this comment.
And maybe even discontiguous slices. pa1.copyFrom(pa2, slice(0, 100, 2))
There was a problem hiding this comment.
Yep. Good ideas. Will do.
|
Question for you. Do you think we should deprecate assignAt as well in favor of the new copyFrom? |
Yep, this sounds good to me. |
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
assignAtbut more flexible. (AndassignAtnow uses_copyFromfor 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.)