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

Add Image.array setter (#1272) #1329

Merged
merged 1 commit into from
Feb 20, 2025
Merged

Add Image.array setter (#1272) #1329

merged 1 commit into from
Feb 20, 2025

Conversation

rmjarvis
Copy link
Member

@sidneymau ran into a weird edge case in a Jupyter notebook when doing im.array /= 10 raised an AttributeError (that array doesn't have a setter), but performed the operation nonetheless.

This seems to be a bug in either Python or numpy. Numpy overloads the inplace arithmetic operations so ar /= 10 is allowed and works in place. But Python seems to think that im.array /= 10 would necessarily require a setter, so it raises an AttributeError. This so far is fine, but then Python calls the numpy __idiv__ operation anyway, so the operation succeeds along with an AttributeError being raised.

It was at least confusing for Sid, since a Jupyter session doesn't die when that happens, so the state of the array seemed inconsistent with the error messaging.

Anyway, this PR adds a setter, which lets im.array = blah work the same way as im.array[:] = blah, with the slight exception that we are careful about rounding things to the nearest integer if we are assigning to integer arrays rather than truncating as numpy would want to do. (This is consistent with how GalSim does other arithmetic on integer Images.)

Note, this replaces Sid's PR #1276, which I wasn't happy with. This implementation never changes the underlying array object. Just lets things be assigned to it as appropriate.

@sidneymau
Copy link
Contributor

Just clarifying that this behavior was not specific to jupyter notebooks but also occurred in any interactive (including pdb/breakpoint) session from what I recall

Copy link
Contributor

@sidneymau sidneymau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks ok to me and tests pass.

@rmjarvis rmjarvis merged commit 967a7d8 into main Feb 20, 2025
10 checks passed
@rmjarvis rmjarvis deleted the #1272 branch February 20, 2025 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants