Skip to content

Changed fill.py example to be less misleading - #11009

Merged
jklymak merged 3 commits into
matplotlib:masterfrom
samvaughan:change-fill-example
Apr 12, 2018
Merged

Changed fill.py example to be less misleading#11009
jklymak merged 3 commits into
matplotlib:masterfrom
samvaughan:change-fill-example

Conversation

@samvaughan

@samvaughan samvaughan commented Apr 9, 2018

Copy link
Copy Markdown

PR Summary

I've changed /examples/lines_bars_and_markers/fill.py to be less misleading, following issues #5827 and #7956.

These plots were over-simplified because fill was acting just like fill_between, since the final line of the region to be filled was completely along the x-axis. I've changed the first example to be a simple polygon (as suggested in #5827) and the second to not have the final point at y=0 anymore. I've also outlined the region being filled, to make what's going on more obvious.

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 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

@samvaughan samvaughan mentioned this pull request Apr 9, 2018
Closed
@tacaswell tacaswell added this to the v2.2-doc milestone Apr 9, 2018
@timhoffm

timhoffm commented Apr 9, 2018

Copy link
Copy Markdown
Member

Thanks for the update.

https://8797-1385122-gh.circle-artifacts.com/0/home/circleci/project/doc/build/html/gallery/lines_bars_and_markers/fill.html?highlight=fill

I don't find the polygon particularly instructive:

  • Start and endpoint are still at the same y.
  • I think we could leave out the grid and thus the zorder.

maybe simply

x = [0, 1, 3, 1]
y = [1, 2, 1, 0]

fig, ax = plt.subplots()
ax.fill(x, y)

Minor things:

  • Single blank lines in the code example are sufficient. No need for two blank lines.
  • For completeness, the first plot should get a plt.plot() as well.

@samvaughan

samvaughan commented Apr 10, 2018

Copy link
Copy Markdown
Author

Sure, no problem. I've removed the grid and made the first example a square

x = [0, 1, 2, 1]
y = [1, 2, 1, 0]
 
fig, ax = plt.subplots()
ax.fill(x, y)

and outlined it in black with plt.plot(). I've removed the double spaces too.

@timhoffm

timhoffm commented Apr 10, 2018

Copy link
Copy Markdown
Member

Sorry, I didn't mean plt.plot() but plt.show(). My bad. The outline does not add extra information in this first example. It's sufficient to illustrate this in the second example.

@samvaughan

samvaughan commented Apr 10, 2018

Copy link
Copy Markdown
Author

No worries, just changed it!

timhoffm
timhoffm approved these changes Apr 11, 2018
View reviewed changes

@timhoffm timhoffm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good

https://circleci.com/gh/matplotlib/matplotlib/8842/artifacts/0//home/circleci/project/doc/build/html/index.html

@jklymak jklymak changed the title Changed fill.py example to be less misleading, after issue #5827 and #7956 Changed fill.py example to be less misleading Apr 12, 2018
@jklymak
jklymak merged commit 640ecd9 into matplotlib:master Apr 12, 2018
lumberbot-app Bot pushed a commit that referenced this pull request Apr 12, 2018
@jklymak MeeseeksDev[bot]
Backport PR #11009: Changed fill.py example to be less misleading
f742f0a
@lumberbot-app lumberbot-app Bot mentioned this pull request Apr 12, 2018
Merged
jklymak added a commit that referenced this pull request Apr 12, 2018
@jklymak
Merge pull request #11038 from matplotlib/auto-backport-of-pr-11009
8ae6058 
Backport PR #11009 on branch v2.2.2-doc
@ImportanceOfBeingErnest ImportanceOfBeingErnest mentioned this pull request Oct 16, 2019
Closed
16 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@timhoffm timhoffm timhoffm approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v2.2-doc

Development

Successfully merging this pull request may close these issues.

4 participants

@samvaughan @timhoffm @tacaswell @jklymak