-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Added get_shape as an alias for get_size + tests #25425
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
Conversation
im = AxesImage(ax) | ||
z = np.arange(12, dtype=float).reshape((4, 3)) | ||
im.set_data(z) | ||
assert im.get_shape() == (4, 3) |
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.
On one hand, this is testing two things so not technically a unit test. On the other hand all of our tests have a high aspect of integration testing to them (e.g. this test invokes plt.gca()
!) so the trade off on one less slightly more complex test seems worth it to me, but fine either way.
assert im.get_shape() == (4, 3) | |
assert im.get_shape() == (4, 3) | |
assert im.get_size() == im.get_shape() |
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.
Left comments about testing, strongly prefer the exception gets tested but won't block merging over that.
I'm fine either way with merging the tests (but have enough of a preference to comment on it).
""" | ||
Return the shape of the image as tuple (numrows, numcols, channels). | ||
""" | ||
if self._A is None: |
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.
You could save having the RuntimeError repeated by making get_size
call get_shape
?
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.
Left comments about testing, strongly prefer the exception gets tested but won't block merging over that.
I'm fine either way with merging the tests (but have enough of a preference to comment on it).
Thanks for the feedback, @jklymak @tacaswell. I decided to merge the tests and add the exception test.
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 don't see that you actually did this....
lib/matplotlib/tests/test_image.py
Outdated
# generate dummy image to test get_shape method | ||
ax = plt.gca() | ||
im = AxesImage(ax) | ||
with pytest.raises(RuntimeError): |
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.
with pytest.raises(RuntimeError): | |
with pytest.raises(RuntimeError, match="You must first set the image array"): |
to make sure it is the "correct" RuntimeError.
lib/matplotlib/image.py
Outdated
@@ -287,10 +287,19 @@ def __getstate__(self): | |||
def get_size(self): | |||
"""Return the size of the image as tuple (numrows, numcols).""" | |||
if self._A is None: |
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.
Maybe just do
return self.get_shape()[:2]
Thanks! |
PR Summary
Fixes #22494. Continuation of draft PR #22510. Added the
get_shape
commit and wrote tests forget_size
method and the newly addedget_shape
method.PR Checklist
Documentation and Tests
pytest
passes)