Skip to content

FIX: remove unnecessary self in super_-calls, fixes #12265 #12268

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 3 commits into from
Sep 29, 2018

Conversation

kmuehlbauer
Copy link
Contributor

@kmuehlbauer kmuehlbauer commented Sep 25, 2018

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@anntzer anntzer added this to the v3.0.x milestone Sep 25, 2018
@anntzer anntzer added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Sep 25, 2018
@anntzer
Copy link
Contributor

anntzer commented Sep 25, 2018

closes #12265

@kmuehlbauer
Copy link
Contributor Author

@anntzer @ImportanceOfBeingErnest I'm not sure how to add tests for this. Would need a bit of guidance.

@ImportanceOfBeingErnest
Copy link
Member

A test would sit either in matplotlib/lib/mpl_toolkits/tests/test_axisartist_axislines.py, matplotlib/lib/mpl_toolkits/tests/test_axisartist_grid_helper_curvelinear.py.

As test you may choose the most minimal, yet general code that would produce this behaviour. Two options are:

  1. Create a "smoketest", i.e. just put the code there. If it throws an error, the test would fail.
  2. Create an image comparisson test. For this you would need to create a target image.

Both are explained in https://matplotlib.org/devel/testing.html

@tacaswell
Copy link
Member

Thanks @kmuehlbauer !

@ImportanceOfBeingErnest
Copy link
Member

Since image comparisson tests are a little expensive, I think one would benefit from creating one single figure with 3 subplots (one for pcolor, one for pcolormesh, one for contour and contourf) instead of 4 individual figures.

In general, you may by now have noticed that axisartist is pretty undertested. Since it seems you are using the axisartist toolkit a lot in your radar image library, there may be other cases you may not want to fail in future versions of matplotlib. Hence, please feel welcome to add additional tests (possibly in future PRs).

@kmuehlbauer
Copy link
Contributor Author

@ImportanceOfBeingErnest You are completely right, I'll change it as suggested as soon as possible.

Yes, we use the toolkits quite extensively. Time to give something back, I think.

@ImportanceOfBeingErnest
Copy link
Member

So what's happening now is that the tests fail (Seen by the red "Some checks were not successful") because of some deviation between the produced image and the target image. This is most often the case when the freetype library in use differs from the one the tests are run with. The two alternative solutions to this are:

  1. Make sure to use the same freetype when compiling matplotlib. This is shown here
  2. Use remove_text=True in the image comparisson decorator. (Which is fine as long as the test isn't really testing anything which requires any texts to be rendered.)

@tacaswell
Copy link
Member

tacaswell commented Sep 25, 2018

As we are already talking about re-generating the images there is also a style option (which should be set to 'default').

Can you please make sure to squash the image commits (so we don't carry around the un-used images in git).

[edited to remove redundant requests]

@kmuehlbauer
Copy link
Contributor Author

@ImportanceOfBeingErnest @tacaswell I'll force push to my branch, to clean the history.

The problem I'm facing, is not the text, but the contour-lines (created from contour) which differ sometimes. Any ideas?

@kmuehlbauer
Copy link
Contributor Author

parasiteaxesauxtrans_meshplot-failed-diff

You see the little dot in the lower right part? That is one corner of the contour line. Only one pixel is different. I know, we have the tol keyword but I'm unsure why this happens. Normally the contourlines should exactly be the same.

@kmuehlbauer
Copy link
Contributor Author

The RMS was below 0.039 in all cases. So I set tol with a safety margin to 0.05 .

@kmuehlbauer
Copy link
Contributor Author

Safety margin wasn't good enough. Will fix this the next day...

@kmuehlbauer
Copy link
Contributor Author

rebased on top of current master due to conflicts

@kmuehlbauer
Copy link
Contributor Author

@anntzer, @ImportanceOfBeingErnest This is good to go from my end. Due to the erratic behaviour of contour I had to set the image comparison tol to 0.075 (it broke last time at 0.052). Let me know, if there is anything to do within this PR.

@QuLogic QuLogic merged commit c15a2b8 into matplotlib:master Sep 29, 2018
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Sep 29, 2018
dstansby added a commit that referenced this pull request Sep 29, 2018
…268-on-v3.0.x

Backport PR #12268 on branch v3.0.x (FIX: remove unnecessary `self` in `super_`-calls, fixes #12265)
@kmuehlbauer kmuehlbauer deleted the fix-12265 branch September 29, 2018 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants