Stock demo Fixed by sirlittle · Pull Request #7529 · matplotlib/matplotlib

sirlittle · 2016-11-29T06:37:18Z

I fixed some of the issues brought up by #7457

tacaswell · 2016-11-29T13:22:29Z

examples/pylab_examples/stock_demo.py

+    """
+    ticker1, ticker2 = 'INTC', 'AAPL'
+
+    file1 = cbook.get_sample_data('INTC.dat.gz')


you need to import cbook

QuLogic

Example does not currently run.

QuLogic · 2016-11-29T21:43:15Z

examples/pylab_examples/stock_demo.py

+
+    file1 = cbook.get_sample_data('INTC.dat.gz')
+    file2 = cbook.get_sample_data('AAPL.dat.gz')
+    M1 = fromstring(file1.read(), '<d')


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.

QuLogic · 2016-11-29T21:43:40Z

examples/pylab_examples/stock_demo.py

+    d1, p1 = M1[:, 0], M1[:, 1]
+    d2, p2 = M2[:, 0], M2[:, 1]
+    return (d1, p1, d2, p2)



Another blank line.

QuLogic · 2016-11-29T21:43:53Z

examples/pylab_examples/stock_demo.py

+same plot. 
+
+"""
+


Another blank line.

QuLogic · 2016-11-29T21:45:40Z

examples/pylab_examples/stock_demo.py

 from matplotlib.ticker import MultipleLocator
-from data_helper import get_two_stock_data
+
+"""


You can put the example's docstring at the beginning of the file; also, add the titling format as indicated here.

QuLogic · 2016-11-29T21:46:52Z

examples/pylab_examples/stock_demo.py


 fig, ax = plt.subplots()
-lines = plt.plot(d1, p1, 's', d2, p2, 'o')
+lines1 = plt.plot(d1, p1, 'b', label="INTC")


Change to ax instead of plt while you're here.

Reportedly does not run, should have just dismissed by review earlier rather than approving.

Fixed.

QuLogic

A couple minor typos.

QuLogic · 2016-11-29T22:40:43Z

examples/pylab_examples/stock_demo.py

+Stock Demo Plots
+================
+
+The following example displays Matplotlibs capabilities of creating


Matplotlib's

QuLogic · 2016-11-29T22:40:55Z

examples/pylab_examples/stock_demo.py

+================
+
+The following example displays Matplotlibs capabilities of creating
+graphs that can be used for stocks. The example specfically uses


specifically

QuLogic · 2016-11-29T22:42:12Z

cc @anntzer who wrote original ticket in case there's anything missing there.

NelleV · 2016-11-29T22:57:46Z

Fixed.

anntzer · 2016-11-30T05:47:22Z

examples/pylab_examples/stock_demo.py

-plt.ylabel('Normalized price')
-plt.xlim(0, 3)
+lines1 = ax.plot(d1, p1, 'b', label="INTC")
+lines2 = ax.plot(d2, p2, 'r', label="AAPL")


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.

NelleV · 2016-11-30T06:28:09Z

Here is the new figure:

efiring · 2016-11-30T06:41:55Z

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.
Overall, we have too many gallery entries, so this would be a good candidate for deletion.
If it is not to be deleted, then I suggest that any revision must at least get rid of the lines across the night periods, and should have a proper date/time abcissa and labels.

codecov-io · 2016-11-30T07:53:06Z

Current coverage is 61.88% (diff: 100%)

Merging #7529 into master will not change coverage

@@             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

Powered by Codecov. Last update b9c3b64...751128c

NelleV · 2016-11-30T16:39:56Z

I am fine with that decision.

anntzer · 2016-11-30T17:02:21Z

Agreed.

tacaswell · 2016-11-30T22:04:09Z

Can this be squashed to a single commit (as the end result is to remove the example)?

NelleV · 2016-11-30T22:05:16Z

I don't feel comfortable force-pushing to a repository that isn't mine. Can we activate the squash and merge button?

QuLogic · 2016-11-30T22:27:16Z

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 master branch. Just a heads up.

QuLogic · 2016-12-05T02:41:49Z

I cherry-picked the deletion onto #7559.

QuLogic · 2016-12-18T01:23:06Z

This is merged now; @sirlittle please remember to reset your master branch before making new changes (and preferably start new features/bugfixes on a separate branch). Please stop by the gitter room if you have any questions about that.

Salil Vanvari added 2 commits November 28, 2016 22:13

fixed style to more greatly represent style

fd14063

added in get_two_stock_demo into file so no more imports

e8c4ef1

tacaswell requested changes Nov 29, 2016

View reviewed changes

Salil Vanvari added 2 commits November 29, 2016 11:11

imported the forgotten cbook

01c3653

Fixed a small bit of formatting

d5a5e2b

tacaswell previously approved these changes Nov 29, 2016

View reviewed changes

tacaswell added the Documentation label Nov 29, 2016

tacaswell added this to the 2.0.1 (next bug fix release) milestone Nov 29, 2016

Added some documentation

751128c

QuLogic previously requested changes Nov 29, 2016

View reviewed changes

FIX the example now works properly + formatting

4954a45

FIX docstring is now at the top

e8e9f75

NelleV changed the title ~~Stock demo Fixed~~ [MRG] Stock demo Fixed Nov 29, 2016

QuLogic reviewed Nov 29, 2016

View reviewed changes

FIX typos

b5efcae

anntzer reviewed Nov 30, 2016

View reviewed changes

ENH plot_stock_demo now uses default colorscheme

1f66e19

NelleV changed the title ~~[MRG] Stock demo Fixed~~ [WIP] Stock demo Fixed Nov 30, 2016

MAINT removing stock_demo.py

589a757

NelleV changed the title ~~[WIP] Stock demo Fixed~~ [MRG] Stock demo Fixed Nov 30, 2016

QuLogic closed this Dec 5, 2016

QuLogic changed the title ~~[MRG] Stock demo Fixed~~ Stock demo Fixed Dec 18, 2016

QuLogic modified the milestones: 2.0 (style change major release), 2.0.1 (next bug fix release) Dec 18, 2016

Uh oh!

Conversation

sirlittle commented Nov 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuLogic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuLogic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuLogic commented Nov 29, 2016

Uh oh!

NelleV commented Nov 29, 2016

Uh oh!

anntzer Nov 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NelleV commented Nov 30, 2016

Uh oh!

efiring commented Nov 30, 2016

Uh oh!

codecov-io commented Nov 30, 2016

Current coverage is 61.88% (diff: 100%)

Uh oh!

NelleV commented Nov 30, 2016

Uh oh!

anntzer commented Nov 30, 2016

Uh oh!

tacaswell commented Nov 30, 2016

Uh oh!

NelleV commented Nov 30, 2016

Uh oh!

QuLogic commented Nov 30, 2016

Uh oh!

QuLogic commented Dec 5, 2016

Uh oh!

QuLogic commented Dec 18, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

anntzer Nov 30, 2016 •

edited

Loading