Stock demo Fixed #7529
Stock demo Fixed #7529
Conversation
| """ | ||
| ticker1, ticker2 = 'INTC', 'AAPL' | ||
|
|
||
| file1 = cbook.get_sample_data('INTC.dat.gz') |
|
|
||
| file1 = cbook.get_sample_data('INTC.dat.gz') | ||
| file2 = cbook.get_sample_data('AAPL.dat.gz') | ||
| M1 = fromstring(file1.read(), '<d') |
There was a problem hiding this comment.
These calls and following lines are missing the np. prefix (because the original data_helper.py imported them poorly.)
I think it can use np.fromfile directly instead of fromstring and a read.
|
|
||
| d1, p1 = M1[:, 0], M1[:, 1] | ||
| d2, p2 = M2[:, 0], M2[:, 1] | ||
| return (d1, p1, d2, p2) | ||
|
|
| same plot. | ||
|
|
||
| """ | ||
|
|
| from matplotlib.ticker import MultipleLocator | ||
| from data_helper import get_two_stock_data 8000 | ||
|
|
||
| """ |
There was a problem hiding this comment.
You can put the example's docstring at the beginning of the file; also, add the titling format as indicated here.
|
|
||
| d1, p1, d2, p2 = get_two_stock_data() | ||
|
|
||
| fig, ax = plt.subplots() | ||
| lines = plt.plot(d1, p1, 's', d2, p2, 'o') | ||
| lines1 = plt.plot(d1, p1, 'b', label="INTC") |
There was a problem hiding this comment.
Change to ax instead of plt while you're here.
Reportedly does not run, should have just dismissed by review earlier rather than approving.
| Stock Demo Plots | ||
| ================ | ||
|
|
||
| The following example displays Matplotlibs capabilities of creating |
| ================ | ||
|
|
||
| The following example displays Matplotlibs capabilities of creating | ||
| graphs that can be used for stocks. The example specfically uses |
|
cc @anntzer who wrote original ticket in case there's anything missing there. |
|
Fixed. |
| plt.ylabel('Normalized price') | ||
| plt.xlim(0, 3) | ||
| lines1 = ax.plot(d1, p1, 'b', label="INTC") | ||
| lines2 = ax.plot(d2, p2, 'r', label="AAPL") |
There was a problem hiding this comment.
I would maintain the default color cycle (so just don't use color specifications).
The straight lines that connect the days also look a bit ugly.
In general it would help if you could post the new figure to the thread.
|
Here is the new figure:
|
|
I'm sorry to be critical of this, but it doesn't serve as a good showcase for matplotlib, and there is nothing stock-specific about it. What is its purpose? In one respect it is worse than the original: it is not appropriate to use a continuous line across the no-data intervals. Using points is the easiest way to avoid this problem. |
Current coverage is 61.88% (diff: 100%)@@ master #7529 diff @@
==========================================
Files 173 173
Lines 56140 56140
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
Hits 34743 34743
Misses 21397 21397
Partials 0 0
|
|
I am fine with that decision. |
|
Agreed. |
|
Can this be squashed to a single commit (as the end result is to remove the example)? |
|
I don't feel comfortable force-pushing to a repository that isn't mine. Can we activate the squash and merge button? |
|
If we squash and merge (which I'm in favour of, because why keep those extraneous changes), no matter if it's via the button or a forced push or a new PR, @sirlittle will have to reset something because this PR is on his |
|
I cherry-picked the deletion onto #7559. |
|
This is merged now; @sirlittle please remember to reset your |
I fixed some of the issues brought up by #7457