Skip to content
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

Adds the gallery example for mplcairo plotting #6835

Merged
merged 40 commits into from Apr 19, 2023
Merged

Conversation

Telomelonia
Copy link
Contributor

@Telomelonia Telomelonia commented Mar 11, 2023

PR Description

This example will go through how to blend two plots from SunPy using mplcairo.

Fixes #6517

TODO

  • Create an example python file in plotting section for this tutorial
  • Check if this is coming in sphinx-gallery docs or not.
  • Changelog.

Issue

  • If the mplcairo is not in local backend then its not working (try something by which this is not required)

@Telomelonia Telomelonia requested a review from a team as a code owner March 11, 2023 13:16
@nabobalis nabobalis marked this pull request as draft March 11, 2023 16:55
@nabobalis nabobalis added Documentation Affects the documentation Examples Affects the Example Gallery labels Mar 11, 2023
@ayshih
Copy link
Member

ayshih commented Mar 11, 2023

The PR is a good start, but you need to add mplcairo as a dependency for building the docs. We need to verify that the example will actually render for our online docs. Until that is verified, there's no point in polishing the example itself.

@Satan-Claws
Copy link
Contributor

The PR is a good start, but you need to add mplcairo as a dependency for building the docs. We need to verify that the example will actually render for our online docs. Until that is verified, there's no point in polishing the example itself.

I've been trying to see if this works, but so far I've been having issues getting mplcairo working on Windows as well as Ubuntu.
For my base environment on Windows it installs correctly and the example works properly (displays and all) but I'm not able to get it working on the sunpy-dev environment (where Sphinx will make docs). I'll try it again on my base environment just to see if it renders correctly or not. If it does, I'll open a new PR and will let you know for changes. I'm still not sure why this happens on non-base environment, mplcairo is a bit buggy.

@nabobalis
Copy link
Contributor

We do not want two open PRs fixing the same issue.

If @Telomelonia and @Satan-Claws can talk to each other and work on this example, that would be better.

@Telomelonia
Copy link
Contributor Author

@Satan-Claws
The main problem is to make mplcairo work for sphinx-gallery to give output ig mplcairo need to be added as a dependency somewhere for it to give output ... else its showing an error.
If you want to work I can close this one.

@Telomelonia
Copy link
Contributor Author

Telomelonia commented Mar 13, 2023

I've been trying to see if this works, but so far I've been having issues getting mplcairo working on Windows as well as Ubuntu. For my base environment on Windows it installs correctly and the example works properly (displays and all) but I'm not able to get it working on the sunpy-dev environment (where Sphinx will make docs). I'll try it again on my base environment just to see if it renders correctly or not. If it does, I'll open a new PR and will let you know for changes. I'm still not sure why this happens on non-base environment, mplcairo is a bit buggy.

Check here and this , it worked for me from the installation method mentioned

@Satan-Claws
Copy link
Contributor

@Telomelonia no no don't close this PR, keep trying and I'll do the same, hopefully one of us gets it soon.

@Satan-Claws
Copy link
Contributor

We do not want two open PRs fixing the same issue.

If @Telomelonia and @Satan-Claws can talk to each other and work on this example, that would be better.

Yeah makes sense

@Satan-Claws
Copy link
Contributor

image

Built docs to check if @Telomelonia plotting built the gallery correctly, and this is the output I got. There are no errors but one plot is missing. I can send the html file if anyone needs.

@Telomelonia
Copy link
Contributor Author

Thanks, @Satan-Claws, silly me ... I ignored the fact that axes and plots should be in one code block.

@ayshih
Copy link
Member

ayshih commented Mar 14, 2023

@Telomelonia In case it wasn't apparent, I've been adding commits to your branch to try to get mplcairo to properly render the example

@Telomelonia
Copy link
Contributor Author

@Telomelonia In case it wasn't apparent, I've been adding commits to your branch to try to get mplcairo to properly render the example

Thanks @ayshih ... I was struggling to work it out.

@ayshih
Copy link
Member

ayshih commented Mar 14, 2023

Please don't change the order of the calls at the start of the example. It is formally required that the matplotlib.use() call comes before the import matplotlib.pyplot as plt line. pre-commit doesn't understand that, so it needs to be told to look the other way in this case.

@ayshih
Copy link
Member

ayshih commented Mar 17, 2023

Okay, the example is working now and all spruced up. @Telomelonia should add a changelog entry. We can wait until sphinx-gallery puts out a new release before accepting this PR so that we don't have to use their development branch. (The reason that the build_docs-gallery build is failing is because I hadn't changed that to also use the sphinx-gallery development branch as I had with RTD.)

@ayshih
Copy link
Member

ayshih commented Apr 14, 2023

I'd still prefer to hold off on merging this PR until the next version of sphinx-gallery is released so that we can remove the use of their development branch.

@nabobalis
Copy link
Contributor

Works for me. Do we have an ETA on a new release?

@nabobalis
Copy link
Contributor

The release came yesterday. We can now remove the git requirement and bump the min version to 0.13.

@nabobalis
Copy link
Contributor

I rebased and tweaked the gallery version and removed the git requirement.

@nabobalis
Copy link
Contributor

NOTE TO MAINAINTERS: SQUASH MERGE

@nabobalis nabobalis requested a review from ayshih April 18, 2023 04:37
@nabobalis nabobalis removed the request for review from a team April 18, 2023 04:37
@nabobalis nabobalis added this to the 5.0.0 milestone Apr 18, 2023
@dstansby dstansby added the No Backport A PR that isn't to be backported to any release branch. (To be used as a flag to other maintainers) label Apr 19, 2023
@dstansby dstansby merged commit b2f733a into sunpy:main Apr 19, 2023
24 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Affects the documentation Examples Affects the Example Gallery No Backport A PR that isn't to be backported to any release branch. (To be used as a flag to other maintainers)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a gallery example showing how to use mplcairo for blending
5 participants