Skip to content

Rectified plot error #12501

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 6 commits into from
Oct 17, 2018
Merged

Rectified plot error #12501

merged 6 commits into from
Oct 17, 2018

Conversation

anubhavshrimal
Copy link
Contributor

@anubhavshrimal anubhavshrimal commented Oct 12, 2018

The plot shown on the page: https://matplotlib.org/tutorials/introductory/usage.html#sphx-glr-tutorials-introductory-usage-py

Will be possible only if plt.show() is outside the loop

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

The plot shown on the page: https://matplotlib.org/tutorials/introductory/usage.html#sphx-glr-tutorials-introductory-usage-py

Will be possible only if plt.show() is outside the loop
@QuLogic
Copy link
Member

QuLogic commented Oct 12, 2018

Thanks for the patch, but that code is intended to be that way (check the paragraph before it.) The image shows three lines because plt.show is patched to do nothing when building docs, but I don't think there is any simple way to work around that.

@anntzer
Copy link
Contributor

anntzer commented Oct 12, 2018

Agreed with @QuLogic, proposing to close the PR.

@ImportanceOfBeingErnest
Copy link
Member

If the image isn't able to show what the code does, especailly if it shows the contrary of what the code does, it should be removed. I also cannot think of another good visualization of this case.

My proposal would hence be to change the relevant part to

# restriction is lifted, so one can write a script like this::
#
#     import numpy as np
#     import matplotlib.pyplot as plt
#
#     plt.ioff()
#     for i in range(3):
#         plt.plot(np.random.rand(10))
#         plt.show()
#
# which makes three plots, one at a time. I.e. the second plot will show up,
# once the first plot is closed.

@anntzer
Copy link
Contributor

anntzer commented Oct 12, 2018

By the way, I think I know how to make this work in sphinx-exhibit, if there's still interest for that...

@ImportanceOfBeingErnest
Copy link
Member

I don't currently see much of a use case for "making this work". In this case, you wouldn't want to clutter the tutorial with 3 images of those plots anyways, right?
In other cases, you would simply put plt.show() at the position where you want to show all figures. If sphinx-exhibit can defer output to the position where plt.show() is called, that may truely be useful. In general I think the only chance sphinx-exhibit has is to come along as a working solution.

@tacaswell
Copy link
Member

I agree with @ImportanceOfBeingErnest 's change. I think that example is meant to show the workflow of

  • plot something
  • show the plot and let the GUI event loop block the rest of python
  • when the user closes the plot window the script continues and opens the next widow
  • repeat

There is no good way to show that in rendered static figures in the docs so it is better to just change it to a code block.

@tacaswell
Copy link
Member

@anubhavshrimal Thanks for your work on this!

@tacaswell tacaswell added this to the v3.1 milestone Oct 12, 2018
@anntzer
Copy link
Contributor

anntzer commented Oct 12, 2018

I don't currently see much of a use case for "making this work". In this case, you wouldn't want to clutter the tutorial with 3 images of those plots anyways, right?

In this specific case I agree

If sphinx-exhibit can defer output to the position where plt.show() is called, that may truely be useful.

That's what I had in mind.

@ImportanceOfBeingErnest
Copy link
Member

Just to make sure how this is moving forwards,
@anubhavshrimal Are you willing to make the changes discussed above to this PR?

@anubhavshrimal
Copy link
Contributor Author

@ImportanceOfBeingErnest Though I would love to work on the discussed changes, I am not really sure how to go about doing that.

@ImportanceOfBeingErnest
Copy link
Member

@anubhavshrimal it should really be as simple as mentionned above

My proposal would hence be to change the relevant part to

# restriction is lifted, so one can write a script like this::
#
#     import numpy as np
#     import matplotlib.pyplot as plt
#
#     plt.ioff()
#     for i in range(3):
#         plt.plot(np.random.rand(10))
#         plt.show()
#
# which makes three plots, one at a time. I.e. the second plot will show up,
# once the first plot is closed.

@anubhavshrimal
Copy link
Contributor Author

anubhavshrimal commented Oct 16, 2018

I have made some changes in the code and docs. Is that what you needed?
I thought it was something with sphinx-exhibit 😅 @ImportanceOfBeingErnest

@ImportanceOfBeingErnest
Copy link
Member

That code block above was meant literally. The idea, as also stated by @tacaswell above

There is no good way to show that in rendered static figures in the docs so it is better to just change it to a code block.

would be to not generate any visual output for this code.

@anubhavshrimal
Copy link
Contributor Author

oh! So what I have understood is that you mean to comment out the rest of the code block as you have done?

@ImportanceOfBeingErnest
Copy link
Member

Yes, exactly.

@anubhavshrimal
Copy link
Contributor Author

Done! Have a look. 👍

@ImportanceOfBeingErnest
Copy link
Member

In principle yes, except you missed the double colon (::)

@ImportanceOfBeingErnest
Copy link
Member

What now happened is that you have a trailing empty space in line 610 and 615. And also the indentation is wrong.

This is your proposed code:

image

This is my proposal from above:

image

@ImportanceOfBeingErnest
Copy link
Member

The remaining error seems to come from

tutorials/introductory/usage.py: E402, E501

which should apparently read

    tutorials/introductory/usage.py: E501

instead. Can you change that as well and see if tests pass. (The codecov error is not important here, but continuous-integration tests need to pass.)

@anubhavshrimal
Copy link
Contributor Author

Sorry but I am not sure what you mean by E501

@ImportanceOfBeingErnest
Copy link
Member

E501 is unimportant. The important part is to remove the E402 from the linked line.

@QuLogic QuLogic merged commit fbeda64 into matplotlib:master Oct 17, 2018
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Oct 17, 2018
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Oct 17, 2018
thoo added a commit to thoo/matplotlib that referenced this pull request Oct 18, 2018
* master: (51 commits)
  Disable sticky edge accumulation if no autoscaling.
  Avoid quadratic behavior when accumulating stickies.
  Rectified plot error (matplotlib#12501)
  endless looping GIFs with PillowWriter (matplotlib#11789)
  TST: add test for re-normalizing image
  FIX: colorbar re-check norm before draw for autolabels
  Fix search for sphinx >=1.8 (matplotlib#12216)
  Fix some flake8 issues
  Don't handle impossible values for `align` in hist()
  DOC: add section about test / doc dependencies
  DOC: clarify time windows for support
  TST: test colorbar tick labels correct
  FIX: make minor ticks formatted with science formatter as well
  DOC: clarify what 'minor version' means
  Adjust the widths of the messages during the build.
  Simplify radar_chart example.
  Fix duplicate condition in pathpatch3d example (matplotlib#12495)
  Fix typo in documentation
  FIX: make unused spines invisible
  Kill FontManager.update_fonts.
  ...
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.

5 participants