-
Notifications
You must be signed in to change notification settings - Fork 108
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
Define setter for image.array to allow inplace operations #1273
Conversation
…cation to address #1254
…sed in ChromaticSum
…otons, which is now unnecessary
image1.array += 1 | ||
image2 = galsim.Image(ref_array.astype(types[i]) + 1) | ||
np.testing.assert_allclose(image1.array, image2.array) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like a few more tests that guarantee that the setter doesn't allocate new memory or change fundamental things about the array. I think the implementation is ok, but there are other ways to have written the setter which would pass this test, but would break the image object.
e.g.
image1.array = 1 # should keep array shape and dtype
image1.array = 1+2j # should only work if array is already complex
image1.array = np.zeros((3,3)) # should only work if current shape is (3,3)
Also, all of those should fail if the image is a cont view.
In other words, try to come up with as many stress tests as possible and make sure it behaves as we would want it to behave.
Also, this shouldn't be directed at releases/2.5. It's not a bugfix. Rebase it to main and direct the PR to main. |
Evidently I don't know how to tell GitHub how to rebase, will try this in a new PR... |
This pull request addresses #1272 by defining a setter for the
Image.array
attribute. This protects against any case where a user does an inplace operation on the image array attribute in an interactive environment: without the setter, this would raise an error while still performing the operation, leading to some confusion. This change also obviates the need to operate on a view of the underling array; e.g., instead ofwe can now do