Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
sidneymau opened this issue Jan 24, 2024 · 9 comments
Labels
feature request Request for a new feature in GalSim notation/conventions Related to choices of units, names of things, other semi-arbitrary conventions

Comments

@sidneymau
Copy link
Contributor

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 sidneymau changed the title Image array throws error about having no setter but still behaves as expected? Image array throws error about having no setter but still performs operation Jan 24, 2024
@sidneymau
Copy link
Contributor Author

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
Copy link
Member

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
psf_image.array[:] /= 10

Both do the same thing and avoid the exception.

@sidneymau
Copy link
Contributor Author

sidneymau commented Jan 24, 2024

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
Copy link
Member

Pull request welcome. ;-)

@sidneymau
Copy link
Contributor Author

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
Copy link
Member

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

@sidneymau
Copy link
Contributor Author

sidneymau commented Jan 25, 2024

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
Copy link
Member

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

@rmjarvis
Copy link
Member

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

rmjarvis added a commit that referenced this issue Feb 23, 2024
@rmjarvis rmjarvis added notation/conventions Related to choices of units, names of things, other semi-arbitrary conventions feature request Request for a new feature in GalSim labels Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request for a new feature in GalSim notation/conventions Related to choices of units, names of things, other semi-arbitrary conventions
Projects
None yet
Development

No branches or pull requests

2 participants