8000 Fix bug in shape assignment by r03ert0 · Pull Request #19619 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]

Skip to content

Fix bug in shape assignment #19619

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

Merged
merged 2 commits into from
Sep 15, 2021
Merged

Fix bug in shape assignment #19619

merged 2 commits into from
Sep 15, 2021

Conversation

r03ert0
Copy link
Contributor
@r03ert0 r03ert0 commented Mar 2, 2021

PR Summary

shape returns first the y dimension, then the x dimension. The code had the order inverted

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

Copy link
@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

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.

@jklymak
Copy link
Member
jklymak commented Mar 2, 2021

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.

@jklymak jklymak added this to the v3.4.1 milestone Mar 2, 2021
@r03ert0
Copy link
Contributor Author
r03ert0 commented Mar 3, 2021

I checked test_streamplot.py and the problem is that the vector field used has a square shape (100x100), so it doesn't matter if dimensions are swapped. Maybe I can just modify this test to have a rectangular shape and make it fail?

@QuLogic
Copy link
Member
QuLogic commented Mar 3, 2021

That seems like the simplest change, yes.

@jklymak
Copy link
Member
jklymak commented Mar 3, 2021

Agreed - a symmetric test is not a very good test

@jklymak
Copy link
Member
jklymak commented Mar 16, 2021

@r03ert0 Any interest in coming back to this?

@QuLogic QuLogic modified the milestones: v3.4.1, v3.4.2 Mar 30, 2021
@jklymak jklymak marked this pull request as draft April 2, 2021 16:04
@QuLogic QuLogic modified the milestones: v3.4.2, v3.4.3 May 4, 2021
@story645
Copy link
Member

hi @r03ert0 do you have the time to come back to this?

@QuLogic
Copy link
Member
QuLogic commented Jul 27, 2021

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 master from square to rectangular tests is here: QuLogic@c2bf8b4

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 master with rectangular tests, and this branch with rectangular tests is here: https://github.com/QuLogic/matplotlib/compare/19619-rectangular..19619-fix-shape

As I mentioned on the call, all of them are different from each other, so I'm not certain which one is correct.

@tacaswell
Copy link
Member

In summary:

  • the bug only shows up when the grid is un-evenly spaced (which is why the tests on the initial PR passed with no change)
  • the bug is in deciding if the gradient has changed too much from point to point (so we should no longer trust the first-order integration), not all of the tests have vector fields with steep enough gradients to trigger this logic
  • The integrated flowlines changed because we are now sampling the underlying function at 2x in one direction which is giving us a better view of the underlying function so it makes sense that the integration will be change.
  • the integrated flow lines would only have been miss-leading in cases with wildly different x/y steps and extremely rapidly changing gradients

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".

@QuLogic QuLogic marked this pull request as ready for review August 5, 2021 07:37
@QuLogic
Copy link
Member
QuLogic commented Aug 5, 2021

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?

@tacaswell tacaswell modified the milestones: v3.4.3, v3.5.0 Aug 5, 2021
@tacaswell
Copy link
Member

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.

@QuLogic
Copy link
Member
QuLogic commented Aug 6, 2021

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.)

10000

shape returns first the y dimension, then the x dimension. The code had the order inverted
@QuLogic
Copy link
Member
QuLogic commented Aug 18, 2021

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.
@jklymak jklymak merged commit c53216b into matplotlib:master Sep 15, 2021
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Sep 15, 2021
timhoffm added a commit that referenced this pull request Sep 15, 2021
…619-on-v3.5.x

Backport PR #19619 on branch v3.5.x (Fix bug in shape assignment)
QuLogic added a commit to QuLogic/cartopy that referenced this pull request Oct 6, 2021
Due to a change [1] after beta1, this image has altered somewhat.

[1] matplotlib/matplotlib#19619
QuLogic added a commit to QuLogic/cartopy that referenced this pull request Oct 6, 2021
Due to a change [1] after beta1, this image has altered somewhat.

[1] matplotlib/matplotlib#19619
QuLogic added a commit to QuLogic/cartopy that referenced this pull request Oct 7, 2021
Due to a change [1] after beta1, this image has altered somewhat.

[1] matplotlib/matplotlib#19619
QuLogic added a commit to QuLogic/cartopy that referenced this pull request Oct 7, 2021
Due to a change [1] after beta1, this image has altered somewhat.

[1] matplotlib/matplotlib#19619
tacaswell pushed a commit that referenced this pull request Oct 20, 2021
Fix bug in shape assignment
ericpre pushed a commit to ericpre/matplotlib that referenced this pull request Oct 20, 2021
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.

7 participants
0