-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
DOC: Update and consolidate Custom Tick Formatter for Time Series example #21873
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you move something, please use the redirect-from directive to indicate where it went, that way old links don't break.
@jklymak Sorry to bother you with that but I can't get the redirect working. I now added the
According to the documentation this should yield the files I cross-checked with other examples and they don't seem to work either. For instance matplotlib/plot_types/README.rst Lines 2 to 4 in 2ccc67f
build/doc/html/tutorials/basic/sample_plots.html but it doesn't exist.Obviously I don't correctly understand the documentation on redirects, so maybe you could lend me a helping hand. |
Perhaps it is broken? |
fe0320d
to
c7bf99d
Compare
Hm, so I just pushed the changes with the |
# Create an index plot (x defaults to range(len(y)) if omitted) | ||
ax2.plot(r.adj_close, 'o-') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just be explicit rather than make the user go back and parse the comment?
# Create an index plot (x defaults to range(len(y)) if omitted) | |
ax2.plot(r.adj_close, 'o-') | |
# Create an index plot: | |
ax2.plot(np.arange(0, len(r.adj_close)), r.adj_close, 'o-') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this change on purpose to underline/explain that this is an index plot which by definition plots the values versus an index number. In my view, the plot command is more legible/clearer without the range definition as the first argument. Besides, this kind of plot is explicitly advertised in plot
, so I'd rather keep it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, however, that most of the core devs consider this a nasty holdover from the Matlab days, and prefer the more explicit interface. Certainly when I was reading this I was confused about what r.adj_close
was, and how it was plotting x and y, and then I had to go up, figure out what r.adj_close
was and then figure out why it didn't need two arguments.
In general, I wonder if it is more confusing than not to keep r
as a structured array? How common do we think those are in the wild (I never use them, but then I use xarray all the time).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
most of the core devs consider this a nasty holdover from the Matlab days, and prefer the more explicit interface
hm, I find this feature quite handy, but if this usage is not recommended it should be described as discouraged in the docs, shouldn't it
I wonder if it is more confusing than not to keep r as a structured array
Alternatives (pandas, xarray, ...) require additional dependencies, csv requires additional date conversion, so why not leave it as a structured array (given that the data structure is described in detail in the comment at lines 24-27)?
- consolidate with Date Index Formatter example and move to Ticks section - use smaller date range to better show the difference to index plotting - visualize missing date section - update sample data description - fix index formatter (empty labels instead of repeated first/last label for out-of-range data)
c7bf99d
to
5ad9546
Compare
Lets merge as an improvement. |
@jklymak Thanks for all your patience with me 👍. |
PR Summary
See #17283 (comment):
PR Checklist
Tests and Styling
pytest
passes).flake8-docstrings
and runflake8 --docstring-convention=all
).Documentation
doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).