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

Always create Axes objects in examples #6822

Merged
merged 10 commits into from Apr 18, 2023
Merged

Conversation

dstansby
Copy link
Member

@dstansby dstansby commented Mar 6, 2023

Our rules of thumb for gallery examples exist to "minimize verbosity and maintain consistency with the other gallery examples:". In some places consistency and verbosity are a trade off, including which flavour of the Matplotlib API we are calling.

Broadly the two options are:

  1. Use pyplot (e.g. plt.xlabel) where possible, and do not create Axes objects

  2. Use Axes all the time, even when it would be possible to use pyplot

  3. is a bit less verbose than 2., but this comes at the cost of having two ways of doing stuff in our gallery (e.g. plt.xlabel() vs. ax.set_xlabel(). In my opinion consistency is more important than minimizing verbosity here, so I am proposing that we always create an Axes object.

I've converted one gallery example as an example of what this change would look like. Thoughts?

@ayshih
Copy link
Member

ayshih commented Mar 6, 2023

I usually favor avoiding the pyplot interface, but I'll point out that the non-pyplot alternative to a plain plt.colorbar() call is notably more clunky.

@Cadair
Copy link
Member

Cadair commented Mar 8, 2023

I am generally in favour of this, it works nicer with WCSAxes and our custom plotting wrappers.

@wtbarnes
Copy link
Member

wtbarnes commented Mar 8, 2023

I don't really have an opinion here as long as we are consistent across the whole gallery.

@dstansby dstansby marked this pull request as draft March 8, 2023 15:19
@dstansby
Copy link
Member Author

dstansby commented Mar 8, 2023

Grand - I'll go through and change everything then, and then take this back out of draft for some proper reviews on the actual implementation.

@dstansby dstansby marked this pull request as ready for review April 14, 2023 08:51
Copy link
Member

@wtbarnes wtbarnes left a comment

Choose a reason for hiding this comment

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

I initially started replacing plt.subplots with fig = plt.figure(); ax = fig.add_subplot() but ran out of steam. I don't particularly like the inconsistency in the ways we create the figure, axes objects, but I do not think it is worth the added verbosity.

docs/dev_guide/contents/example_gallery.rst Outdated Show resolved Hide resolved
Co-authored-by: Will Barnes <will.t.barnes@gmail.com>
@nabobalis nabobalis added this to the 5.0.0 milestone Apr 18, 2023
@nabobalis nabobalis merged commit ff99474 into sunpy:main Apr 18, 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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants