Skip to content

timeline example #11403

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 2 commits into from
Jun 9, 2018
Merged

timeline example #11403

merged 2 commits into from
Jun 9, 2018

Conversation

choldgraf
Copy link
Contributor

PR Summary

This is a short example to demonstrate how to make a timeline with Matplotlib. @NelleV said it might be useful for the docs since it blends a few different parts of Matplotlib functionality.

Here's what the plot looks like:

image

PR Checklist

  • Code is PEP 8 compliant
  • Documentation is sphinx and numpydoc compliant

@tacaswell tacaswell added this to the v2.2-doc milestone Jun 9, 2018
@tacaswell
Copy link
Member

I like it!

@tacaswell
Copy link
Member

should probably remove the betas from 2.0 as well?

@choldgraf
Copy link
Contributor Author

arg it looks like Pandas isn't installed in circle? Should I figure out a way to do this w/o pandas?

@choldgraf
Copy link
Contributor Author

also, updated removing the betas:

image


# Grab a list of Matplotlib releases
url = 'https://api.github.com/repos/matplotlib/matplotlib/releases'
data = json.loads(urllib.request.urlopen(url).read().decode())

Choose a reason for hiding this comment

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

I'm not sure if the examples should depend on web links. Or is this safe to link to github.com because the tests themselves run on github?

@ImportanceOfBeingErnest
Copy link
Member

Pandas is neither a dependency of matplotlib, nor of the documentation. You may append _sgskip to the filename of your example such that it will not be run (e.g. timeline_sgskip.py).
The drawback of this is that it would hence not produce an output image in the docs, such that one would need to statically link an image instead. This however would require to recreate this static image manually for each release to keep it up to date. On the other hand it would also solve the problem of scaping a web resoure in the docs built.

# Note that Matplotlib will automatically plot datetime inputs.

levels = np.array([-5, 5, -3, 3, -1, 1])
fig, ax = plt.subplots(figsize=(20, 5))
Copy link
Member

Choose a reason for hiding this comment

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

The maximal figure width an example should have is 900 pixels (=9 inch for default dpi). See this issue.

@ImportanceOfBeingErnest
Copy link
Member

ImportanceOfBeingErnest commented Jun 9, 2018

There is also already a stem example. I wonder in how far this could be added to it instead of creating a new one (and essentially reinventing the stem). Or maybe this is better suited for the showcase section of the gallery?

Here is how I would do it with stem, without the use of pandas, and with matplotlib.dates locators and formatters instead of fixed tick locations.

import matplotlib.pyplot as plt
import matplotlib.dates as mdates
import numpy as np
from datetime import datetime
import urllib.request
import json

# Grab a list of Matplotlib releases
url = 'https://api.github.com/repos/matplotlib/matplotlib/releases'
data = json.loads(urllib.request.urlopen(url).read().decode())

dates = []
release = []
for item in data:
    if 'rc' not in item['tag_name'] and 'b' not in item['tag_name']:  
        dates.append(item['published_at'])
        release.append(item['tag_name'])

# Convert date strings (e.g. 2014-10-18T18:56:23Z) to datetime      
dates = [datetime.strptime(d, "%Y-%m-%dT%H:%M:%SZ") for d in dates]

# Choose some nice levels
levels = np.tile([-5, 5, -3, 3, -1, 1], 
                 int(np.ceil(len(dates)/6)))[:len(dates)]

# Create figure and plot a stem plot with the date
fig, ax = plt.subplots(figsize=(9, 5))
ax.set(title="Matplotlib release dates")

markerline, stemlines, baseline = ax.stem(dates, levels,
                                          basefmt="k-", linefmt="C3-")

plt.setp(markerline, mec="k", mfc="w", zorder=3)
markerline.set_ydata(np.zeros(len(dates)))


# annotate lines
vert = np.array(['top', 'bottom'])[(levels > 0).astype(int)]
for d, l, r, va in zip(dates, levels, release, vert):
    ax.annotate(r, xy=(d,l), xytext=(-3,np.sign(l)*3),
                textcoords="offset points", va=va, ha="right")

# format xaxis with 3 month intervals
ax.get_xaxis().set_major_locator(mdates.MonthLocator(interval=3))
ax.get_xaxis().set_major_formatter(mdates.DateFormatter("%b %Y"))
fig.autofmt_xdate()

# remove y axis and spines
ax.get_yaxis().set_visible(False)
for spine in ["left", "top", "right"]:
    ax.spines[spine].set_visible(False)

ax.margins(y=0.1)
plt.show()

image

@choldgraf
Copy link
Contributor Author

stem plots are fine w/ me too, I can replace this example with the one using stem plots if you prefer...wdyt @tacaswell ?

@NelleV
Copy link
Member

NelleV commented Jun 9, 2018

@choldgraf Thanks a lot for your contribution! It's a really nice addition to the gallery.

I think a good first step is to remove the pandas dependencies.

@choldgraf
Copy link
Contributor Author

ok, latest push removes the dependency on pandas - here's what the output looks like. WDYT @NelleV ?

image

@NelleV
Copy link
Member

NelleV commented Jun 9, 2018

I think this looks great! Thanks a lot for this contribution, @choldgraf !

@NelleV NelleV merged commit cd1ae80 into matplotlib:master Jun 9, 2018
lumberbot-app bot pushed a commit that referenced this pull request Jun 9, 2018
@ImportanceOfBeingErnest
Copy link
Member

Did anyone read my comments???? Can they please be answered and discussed before something is just merged?

This PR introduces the first and only case where the doc build requires a web connection. This is a major change. If no web connection can be established, or the server is unavailable, the doc built would fail.
Is this desired?

Apart, I'm really not a big fan of creating plots in a loop - this is inefficient and should not be advertised. (But, that is sure my personal opinion only - still it would be good to have a third one on this.)

@jklymak
Copy link
Member

jklymak commented Jun 9, 2018

I agree that the URL pull should at least have a try except around it. Not sure if the actual pull is problematic otherwise.

I do think the figure is misscaled and should be 9” wide at most. Even if printed in landscape the fonts would be 6 pt on a letter-sized page (or A4) and that is too small to be readable.

Not sure I understand the issue with the loop since the annotation has to be in a loop anyhow.

However I also agree that this is a good use of stem and the example somewhat duplicates stem. A modification could show both methods?

I expect all this could be dealt with via a follow up PR rather than bothering to revert this one. We hear from @choldgraf so rarely it’d be nice not to harass him into hiding 😉

@NelleV
Copy link
Member

NelleV commented Jun 10, 2018

@ImportanceOfBeingErnest I'm sorry if it seemed I was dismissing your comments, but all of these can be addressed in following PRs. It is overall an improvement to our documentation, and it is our guideline to merge when documentation pull requests are an improvement to our documentation. We have set those guidelines in order to encourage documentation pull request: there is nothing more discouraging than long-lasting (and sometimes bikeshedding) comments on documentation pull requests.

I agree that pulling from the web is a problem that should be addressed. It is also an easy fix, that can be done either by a core contributor, or a first-time contributor, and if we apply our reviewing guidelines, it is a pull request that should in theory get into matplotlib fairly smoothly.

@choldgraf
Copy link
Contributor Author

@ImportanceOfBeingErnest I fixed the pandas dependency but I missed your web pulling comment, sorry about that. I think you make a good point there. How about I open up a clean-up PR that does something similar to https://matplotlib.org/gallery/showcase/firefox.html#sphx-glr-gallery-showcase-firefox-py and fixes the figsize

@choldgraf
Copy link
Contributor Author

see #11410 !

@ImportanceOfBeingErnest
Copy link
Member

The most important part is to get rid of the web dependency. As @tacaswell suggested on gitter you may use a try-except to pull from the online resource and in case that fails provide a fall-back dataset.
I think it's appealing that this example would update itself for every new release automatically.
But of course providing the complete dataset within the code (as is done with the firefox logo) is equally possible.

Second, the figure size should be fixed, indeed.

For everything else there are some comments here about usage of stem and other possible improvements, like linking between examples or comparisson between methods. Those are for sure no road-blockers and I agree that those are the kind of things, which if missing, one would not disqualify this as an "overall improvements to our documentation" as @NelleV put it.


@NelleV I have no idea what the reviewing guidelines say about introducing dependencies, but I doubt that they should allow for two-eyes-only reviews - even if it's only the docs. I would understand your point if you simply did not see the implications in the code. But given that there was a clear warning about this???

So I would suggest to include a sentence in your guidelines that says something like "Do never unilaterally merge any PR for which there is an indication that it could hinder the successful build of library or its documentation."

@choldgraf
Copy link
Contributor Author

@ImportanceOfBeingErnest the URL + figsize issues are in #11410

also re: review guidelines

from https://matplotlib.org/devel/coding_guide.html#pr-review-guidelines :

image

@ImportanceOfBeingErnest
Copy link
Member

...or possibly add "be responsible and use your common sense" as an item in that list.

@choldgraf
Copy link
Contributor Author

seems reasonable! it's worth noting that the point of having different rules for documentation is that these changes usually won't "break" anything, are often a first point-of-contact for new contributors, and are particularly susceptible to small differences in opinion dragging PRs on. In this case I think you're right to point out the potential pitfall of relying on the web for one of the examples. I should have addressed that in the original PR, though as mentioned it is now in #11410

@NelleV
Copy link
Member

NelleV commented Jun 10, 2018

There are guidelines about adding dependencies, but this PR added none (urllib, json, and datetime are all part of the standard library, and all three already fairly extensively used in the code base).

Again, if you are unhappy with the reviewer guidelines, speak with other core developers to change them. I don't think this is the place for having such discussion. Maybe the monday meeting is a better place?

@jklymak
Copy link
Member

jklymak commented Jun 10, 2018

The guidelines are fine. This is hardly a big deal and can be easily fixed. These things tend to be self-correcting, and if this had not been caught at the review stage, it would have been an easy fix after someone tried to build docs offline.

That said, if someone takes the time to review something, if we merge, we should be certain to engage with the review carefully, even if we don’t agree with the review or think some of its points could be separate PRs.

@choldgraf
Copy link
Contributor Author

I agree @jklymak - I apologize that I came across as uninterested in engaging with the review. I'll make an effort to read and communicate more carefully in the future!

@jklymak
Copy link
Member

jklymak commented Jun 10, 2018

No apologies necessary at all! We are all busy and just doing this for fun.

jklymak added a commit that referenced this pull request Jun 13, 2018
…n-v2.2.2-doc

Backport PR #11403 on branch v2.2.2-doc
jklymak added a commit that referenced this pull request Jun 20, 2018
dstansby added a commit that referenced this pull request Jun 21, 2018
…of-pr-11403-on-v2.2.2-doc

Revert "Backport PR #11403 on branch v2.2.2-doc"
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