pupil_u, pupil_v being modified in RubinOptics.applyTo

[esheldon]

I noticed that the pupil positions of the photon array are being modifed by RubinOptics.applyTo

Here "x, y" are references to the pupil values, not copies

https://github.com/LSSTDESC/imSim/blob/b6a633f03336f2e0afcd4009558fefb240e118e3/imsim/photon_ops.py#L92

subsequently these get modified in place in the trace call.

I suggest that those x, y should be copies of the pupil values rather than references

[rmjarvis]

I think I would probably consider this a bug in batoid.RayVector._directInit. That function shouldn't modify it's input parameters. @jmeyers314 Would this be an easy fix to make? Or should we add a line to make copies in imsim?

[jmeyers314]

I think I'd probably just make copies on the line @esheldon highlighted. A bunch of batoid operates in-place by design b/c I noticed it made a noticeable speed difference.

As an aside, supposedly the public RayVector ctor makes copies, so we could theoretically switch to that: https://github.com/jmeyers314/batoid/blob/07edb511630c57131d9072494e7855571865e3fa/batoid/rayVector.py#L23-L25.

Though now I'm looking at the docs for np.ascontiguousarray and see that it only copies if needed, so the above docstring might be a lie.

Weird:

In [13]: arr = np.arange(10)

In [14]: arr1 = np.ascontiguousarray(arr)

In [15]: arr1.data == arr.data
Out[15]: True

In [16]: arr1.data
Out[16]: <memory at 0x105859b40>

In [17]: arr.data
Out[17]: <memory at 0x11fe374c0>

In [18]: arr1 is arr
Out[18]: True

So the str() prints out different addresses but they're actually the same?

[rmjarvis]

I think these are two different pointers to the same place in memory. The hex value you see here is the address of the pointer. Not the address it points to. Check either arr.ctypes.data or arr.__array_interface__['data']. These should match if the arrays point to the same place in memory.

[rmjarvis]

And I take back the comment that this is a bug in batoid. It's a leading underscore method, which (in my framework of UI design) is allowed to take this kind of shortcut. So I agree it's on us to use that method properly. There may still be a bug in the non-underscore version, but that's a separate matter.

[jmeyers314]

Addressed by #477 .