GalSim-developers / GalSim

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

Image array throws error about having no setter but still performs operation #1272

Open sidneymau opened 10 months ago

sidneymau commented 10 months ago

I guess this only works in an interactive session / debug shell, but modifying the underlying numpy array of an image can be done despite an AttributeError being thrown. For example, the following throws an AttributeError but the operation still has taken place. This is somewhat confusing behavior

(Pdb) psf_image.array
array([[-4.3576780e-13, -4.2518140e-13, -4.0360353e-13, ...,
         1.5450338e-13, -1.7318126e-14, -2.6135360e-13],
       [-4.2518140e-13, -5.3816332e-13, -5.9797236e-13, ...,
         4.2668004e-13,  1.6594023e-13, -1.3191374e-13],
       [-4.0360353e-13, -5.9797236e-13, -7.3459999e-13, ...,
         6.1851040e-13,  3.0497444e-13, -7.1951275e-14],
       ...,
       [ 1.5450338e-13,  4.2668004e-13,  6.1851040e-13, ...,
        -3.1695967e-13, -1.8824241e-13, -9.3841832e-14],
       [-1.7318127e-14,  1.6594022e-13,  3.0497444e-13, ...,
        -1.8824241e-13, -2.0354703e-13, -2.3963579e-13],
       [-2.6135357e-13, -1.3191373e-13, -7.1951275e-14, ...,
        -9.3841818e-14, -2.3963579e-13, -4.1648504e-13]], dtype=float32)
(Pdb) psf_image.array /= 10
*** AttributeError: property 'array' of 'Image' object has no setter
(Pdb) psf_image.array
array([[-4.3576779e-14, -4.2518140e-14, -4.0360354e-14, ...,
         1.5450338e-14, -1.7318125e-15, -2.6135361e-14],
       [-4.2518140e-14, -5.3816333e-14, -5.9797233e-14, ...,
         4.2668004e-14,  1.6594023e-14, -1.3191374e-14],
       [-4.0360354e-14, -5.9797233e-14, -7.3459996e-14, ...,
         6.1851037e-14,  3.0497445e-14, -7.1951273e-15],
       ...,
       [ 1.5450338e-14,  4.2668004e-14,  6.1851037e-14, ...,
        -3.1695966e-14, -1.8824240e-14, -9.3841832e-15],
       [-1.7318128e-15,  1.6594023e-14,  3.0497445e-14, ...,
        -1.8824240e-14, -2.0354703e-14, -2.3963579e-14],
       [-2.6135357e-14, -1.3191373e-14, -7.1951273e-15, ...,
        -9.3841815e-15, -2.3963579e-14, -4.1648505e-14]], dtype=float32)
sidneymau commented 10 months ago

Here's my understanding—do feel free to correct me. I think this is because array is made a property of Image via the @property decorator, so no __setter__ is defined. However, instead of changing the property, we are modifying the array in place with the /= operator, so Image.array doesn't need a setter for this operation to occur. The discussion of this stack overflow exchange covers this: https://stackoverflow.com/q/60810463.

If the intention was for Image.array to be read-only but have Image.array[:] be writable, then maybe a solution would be to set the flags for Image.array to be unwriteable (Image.array.flags["WRITABLE"] = False) and add a setter that uses a view of the original array for any modifications?

rmjarvis commented 10 months ago

Honestly, I think this is a Python bug. Python thinks that something using /= requires a setter, but numpy has overridden that to work in place. You can check that the id of the array before and after is the same, so you didn't assign a new object to psf_image.array. Rather, numpy modified the elements in place.

And obviously, the intent is very much NOT to have the array to be read-only. We modify Image arrays all the time. In fact that's basically the whole point of most of GalSim's functionality.

I'm not sure if there is a strong need to fix this. The current workarounds are pretty easy. 1.

ar = psf_image.array
ar /= 10

2.

psf_image.array[:] /= 10

Both do the same thing and avoid the exception.

sidneymau commented 10 months ago

Yes, I agree it is easy to avoid this problem. I mostly raised this issue because I ran into a situation where I forgot to make the view of the array and received the error (e.g., psf_image.array /= 10) and so assumed the operation failed, but was confused when the image array values had indeed changed. This is fine in a script/batch environment where the error will cause python to exit, but may lead to some problems for, e.g., users running analyses in notebooks.

Here's a quick mockup that seems to alleviate this behavior:

import numpy as np

class Image:
    def __init__(self, array):
        self._array = array

    @property
    def array(self):
        return self._array

    @array.setter
    def array(self, other):
        self._array[:] = other

if __name__ == "__main__":
    array = np.zeros((2, 2))
    image = Image(array)

    array_id = id(array)

    image_array_id = id(image.array)

    image.array += 1

    new_image_array_id = id(image.array)

    assert array_id == image_array_id
    assert image_array_id == new_image_array_id

    np.testing.assert_array_equal(image.array, np.ones((2, 2)))
rmjarvis commented 10 months ago

Pull request welcome. ;-)

sidneymau commented 10 months ago

While testing, I've also found that

image = galsim.Image(np.zeros((2, 2)))
image += 1
np.testing.assert_array_equal(image.array, np.ones((2, 2)))

also works fine. Will still submit a pull request to prevent the unnecessary error from popping up when modifying image.array in place

rmjarvis commented 10 months ago

image += 1 That seems correct, no? Was this surprising behavior from your perspective? Looks to me like that is working as intended.

sidneymau commented 10 months ago

Surprising in the sense that I don't recall having seen it documented. I always assumed that operations of the array values of an image had to happen on the underlying array and were not possible on the image itself (especially because most numpy operations don't work on the image object but do on the array).

For example,

>>> image = galsim.Image(np.zeros((2, 2)))
>>> np.sum(image)
galsim.Image(bounds=galsim.BoundsI(xmin=1, xmax=2, ymin=1, ymax=2), array=
array([[0., 0.],
       [0., 0.]]), wcs=None)
>>> np.sum(image.array)
0.0
rmjarvis commented 10 months ago

It looks like they may have escaped documentation, but here is the code and tests tests of this functionality.

rmjarvis commented 10 months ago

Calling numpy functions on images directly doesn't seem like an appropriate API. But arithmetic seems quite intuitive to me. YMMV.