Fix bug in shape assignment #19619
Fix bug in shape assignment #19619
Conversation
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
|
That does seem to be incorrect, thanks for the PR. Any chance you can add a test for this? I don't understand why no tests were failing or no tests were "broken" by this fix. |
|
I checked |
|
That seems like the simplest change, yes. |
|
Agreed - a symmetric test is not a very good test |
|
@r03ert0 Any interest in coming back to this? |
|
hi @r03ert0 do you have the time to come back to this? |
|
I changed the test as so: diff --git a/lib/matplotlib/tests/test_streamplot.py b/lib/matplotlib/tests/test_streamplot.py
index 3ed1118817..74cf4f4003 100644
--- a/lib/matplotlib/tests/test_streamplot.py
+++ b/lib/matplotlib/tests/test_streamplot.py
@@ -13,14 +13,14 @@ on_mac = (sys.platform == 'darwin')
def velocity_field():
- Y, X = np.mgrid[-3:3:100j, -3:3:100j]
+ Y, X = np.mgrid[-3:3:100j, -3:3:200j]
U = -1 - X**2 + Y
V = 1 + X - Y**2
return X, Y, U, V
def swirl_velocity_field():
- x = np.linspace(-3., 3., 100)
+ x = np.linspace(-3., 3., 200)
y = np.linspace(-3., 3., 100)
X, Y = np.meshgrid(x, y)
a = 0.1
@@ -70,8 +70,8 @@ def test_linewidth():
def test_masks_and_nans():
X, Y, U, V = velocity_field()
mask = np.zeros(U.shape, dtype=bool)
- mask[40:60, 40:60] = 1
- U[:20, :20] = np.nan
+ mask[40:60, 80:120] = 1
+ U[:20, :40] = np.nan
U = np.ma.array(U, mask=mask)
# Compatibility for old test image
ax = plt.figure().subplots()The diff (for PNGs only) of The diff (for PNGs only) of this branch (merged to master first) from square to rectangular tests is here: QuLogic@19f2239 And the diff between As I mentioned on the call, all of them are different from each other, so I'm not certain which one is correct. |
|
In summary:
The images in QuLogic/matplotlib@19f2239 are the "most right". We probably should also add an API change note to the effect of "computed flow lines may change for streamplot with un-even grids". |
Should we still backport to 3.4.3 then, or stick to 3.5? |
|
I think stick to v3.5. Given that this is a very old bug, I do not think it is urgent to get in for 3.4.3 and given that it does change outputs waiting for 3.5 seems safer. |
|
OK, if people are okay with those images, I will update the SVG and PDF (or possibly just increase the tolerance on the one or two that are just bit changes.) |
shape returns first the y dimension, then the x dimension. The code had the order inverted
|
Alright, I've updated all the formats now. For those where changes were not visible to a viewer, I only updated the tolerance. For those that were updated, I also dropped the back-compat code in the tests, so they are slightly different from what they were before. |
Wherein test images needed changing, the compatibility code for the original images has also been removed.
…619-on-v3.5.x Backport PR #19619 on branch v3.5.x (Fix bug in shape assignment)
Due to a change [1] after beta1, this image has altered somewhat. [1] matplotlib/matplotlib#19619
Due to a change [1] after beta1, this image has altered somewhat. [1] matplotlib/matplotlib#19619
Due to a change [1] after beta1, this image has altered somewhat. [1] matplotlib/matplotlib#19619
Due to a change [1] after beta1, this image has altered somewhat. [1] matplotlib/matplotlib#19619
Fix bug in shape assignment
Fix bug in shape assignment
PR Summary
shapereturns first the y dimension, then the x dimension. The code had the order invertedPR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).