[MRG] Updated plot_stock_market.py to use Google Finance #9010

superbobry · 2017-06-06T14:21:35Z

Reference Issue

What does this implement/fix? Explain your changes.

This PR replaces a dependency on quotes_historical_yahoo_ochl
with a custom wrapper over Google Finance API.

The implementations is intentionally very basic not to distract the users
from the example. Specifically unlike quotes_historical_yahoo_ochl it
does not cache downloaded data.

Any other comments?

I had to remove some symbols because the have no data on Google for
the specified date interval. These are WBA, LMT, KFT and MTU.

The implementations is intentionally very basic not to distract the users from the example. Specifically unlike ``quotes_historical_yahoo_ochl`` it does not cache downloaded data. I also had to remove some symbols because the have no data on Google for the specified date interval. These are WBA, LMT, KFT and MTU. Closes scikit-learn#8899

lesteve · 2017-06-06T14:39:13Z

Nice thanks for picking this up!

lesteve · 2017-06-06T14:40:10Z

examples/applications/plot_stock_market.py

+        'output': 'csv'
+    })
+    url = 'http://www.google.com/finance/historical?' + params
+    print(url)


I would not print the url.

Yep, this is leftover debugging code. Will remove.

lesteve · 2017-06-06T14:40:51Z

doc/conf.py

@@ -241,8 +241,7 @@
        'matplotlib': 'http://matplotlib.org',
        'numpy': 'http://docs.scipy.org/doc/numpy-1.8.1',
        'scipy': 'http://docs.scipy.org/doc/scipy-0.13.3/reference'},
-    'expected_failing_examples': [
-        '../examples/applications/plot_stock_market.py']
+    'expected_failing_examples': []


I think you can remove the expected_failing_examples but please double-check

Done. The key was added in this commit: 9759019.

lesteve · 2017-06-06T14:50:18Z

Here are the plots, not sure I can comment in detail about the differences:

from stable doc:

this PR (run locally on my laptop):

lesteve · 2017-06-06T14:56:15Z

I edited your title to have [MRG] at the beginning of your title. You can have a look at our contribution guidelines

lesteve · 2017-06-06T14:57:17Z

@GaelVaroquaux since you are the author of the example, maybe you can have a quick look at the differences in the plots?

superbobry · 2017-06-06T15:05:34Z

I think the differences are expected, since I had to change the data set (Google does not have the same data). However, I cannot comment whether the new plot makes sense.

vene · 2017-06-06T15:33:59Z

examples/applications/plot_stock_market.py

@@ -131,9 +161,7 @@
    'CSCO': 'Cisco',
    'TXN': 'Texas instruments',


while we're at it, the i should probably be uppercase, same for the 'e' in American Express, and I think "Mc Donalds" should be "McDonald's".

vene · 2017-06-06T15:37:29Z

examples/applications/plot_stock_market.py

-open = np.array([q.open for q in quotes]).astype(np.float)
-close = np.array([q.close for q in quotes]).astype(np.float)
+close = np.stack([q['close'] for q in quotes])
+open = np.stack([q['open'] for q in quotes])

 # The daily variations of the quotes are what carry most information
 variation = close - open


Again, not this pr's fault, but open is a reserved keyword in python, could we rename these close_quote / open_quote or whatever the explicit finance terms are?

close_prices/open_prices seems fine with me.

vene · 2017-06-06T15:41:52Z

examples/applications/plot_stock_market.py

-except ImportError:
-     # quotes_historical_yahoo_ochl was named quotes_historical_yahoo before matplotlib 1.4
-     from matplotlib.finance import quotes_historical_yahoo as quotes_historical_yahoo_ochl
+from matplotlib import pyplot as plt


any reason for changing this import from how it was?

We have it both ways in the codebase, but overwhelmingly more common in the first way. There should be no difference so it's not a big deal, just curious why you changed it.

Just a matter of personal preference. I'll revert it back, it is unrelated to the PR.

vene · 2017-06-06T15:46:46Z

The code itself and the new plot look good to me. We still have similar clusters, e.g., most of the defense contractors.

I think the only question is whether we need to cache the data or not, probably only for the sake of CI. If so, we might as well add the downloader to the datasets module...

lesteve · 2017-06-06T15:54:13Z

I think the only question is whether we need to cache the data or not, probably only for the sake of CI. If so, we might as well add the downloader to the datasets module...

The example takes ~20s to run on my laptop, including download, so my take on it is not to bother about these aspects.

lesteve · 2017-06-06T16:29:33Z

The example takes ~20s to run on my laptop, including download, so my take on it is not to bother about these aspects.

And BTW thinking about it a bit more the data was not cached previously by the CIs since IIRC it was downloaded inside ~/.matplotlib somewhere.

vene · 2017-06-06T16:30:08Z

very good points. For simplicity let's leave it like this then.

jnothman · 2017-06-07T02:36:23Z

Not sure why Circle failed. Rebuilding. Cancelling travis and appveyour as irrelevant.

agramfort · 2017-06-07T05:31:07Z

Circle is happy

thx @superbobry

i'll fix the import matplotlib.pyplot as plt in master directly

…n#9010) * DOC updated plot_stock_market.py to use Google Finance The implementations is intentionally very basic not to distract the users from the example. Specifically unlike ``quotes_historical_yahoo_ochl`` it does not cache downloaded data. I also had to remove some symbols because the have no data on Google for the specified date interval. These are WBA, LMT, KFT and MTU. Closes scikit-learn#8899 * DOC removed plot_stock_market.py from expected failing examples * Addressed review comments * Addressed another pass of review comments

* Fix tests on numpy master (#7946) Until now we were in a edge case on assert_array_equal * Fix tests on numpy master (#8355) numpy.apply_along_axis has changed behaviour when the function passed in returns a 2d array * [MRG] Updated plot_stock_market.py to use Google Finance (#9010) * DOC updated plot_stock_market.py to use Google Finance The implementations is intentionally very basic not to distract the users from the example. Specifically unlike ``quotes_historical_yahoo_ochl`` it does not cache downloaded data. I also had to remove some symbols because the have no data on Google for the specified date interval. These are WBA, LMT, KFT and MTU. Closes #8899 * DOC removed plot_stock_market.py from expected failing examples * Addressed review comments * Addressed another pass of review comments * [MRG] Remove DeprecationWarnings in examples due to using floats instead of ints (#8040)

amueller · 2017-06-20T16:17:07Z

examples/applications/plot_stock_market.py


-open = np.array([q.open for q in quotes]).astype(np.float)
-close = np.array([q.close for q in quotes]).astype(np.float)
+close_prices = np.stack([q['close'] for q in quotes])


stack was introduced in numpy 1.10, not present in our minimum requirement 1.8.2

amueller · 2017-06-20T16:17:18Z

examples/applications/plot_stock_market.py

+        'output': 'csv'
+    })
+    url = 'http://www.google.com/finance/historical?' + params
+    with urlopen(url) as response:


this doesn't work in python2.7

…n#9010) * DOC updated plot_stock_market.py to use Google Finance The implementations is intentionally very basic not to distract the users from the example. Specifically unlike ``quotes_historical_yahoo_ochl`` it does not cache downloaded data. I also had to remove some symbols because the have no data on Google for the specified date interval. These are WBA, LMT, KFT and MTU. Closes scikit-learn#8899 * DOC removed plot_stock_market.py from expected failing examples * Addressed review comments * Addressed another pass of review comments

Sergei Lebedev added 2 commits June 6, 2017 16:06

DOC removed plot_stock_market.py from expected failing examples

bef50e0

lesteve reviewed Jun 6, 2017

View reviewed changes

lesteve changed the title ~~Updated plot_stock_market.py to use Google Finance~~ [MRG] Updated plot_stock_market.py to use Google Finance Jun 6, 2017

Addressed review comments

2dc9cbd

vene reviewed Jun 6, 2017

View reviewed changes

Addressed another pass of review comments

c1d1541

agramfort merged commit 2c55551 into scikit-learn:master Jun 7, 2017

jakirkham mentioned this pull request Jun 15, 2017

Backport NumPy 1.13.0 fixes to 0.18.X #9137

Merged

amueller mentioned this pull request Jun 20, 2017

fix plot_stock_market.py to work on python2.7 and numpy1.6 #9181

Merged

amueller reviewed Jun 20, 2017

View reviewed changes

superbobry mentioned this pull request Jul 19, 2017

stock analysis example gives urllib2.HTTPError: 404 for yahoo API call hmmlearn/hmmlearn#207

Closed

superbobry mentioned this pull request Oct 8, 2017

Gaussian HMM Jupyter Notebook and code examples are not working hmmlearn/hmmlearn#222

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] Updated plot_stock_market.py to use Google Finance #9010

[MRG] Updated plot_stock_market.py to use Google Finance #9010

superbobry commented Jun 6, 2017

lesteve commented Jun 6, 2017

lesteve Jun 6, 2017

superbobry Jun 6, 2017

lesteve Jun 6, 2017

superbobry Jun 6, 2017 •

edited

Loading

lesteve commented Jun 6, 2017

lesteve commented Jun 6, 2017

lesteve commented Jun 6, 2017

superbobry commented Jun 6, 2017

vene Jun 6, 2017

superbobry Jun 6, 2017

vene Jun 6, 2017

lesteve Jun 6, 2017

superbobry Jun 6, 2017

vene Jun 6, 2017

superbobry Jun 6, 2017

vene commented Jun 6, 2017

lesteve commented Jun 6, 2017 •

edited

Loading

lesteve commented Jun 6, 2017

vene commented Jun 6, 2017

jnothman commented Jun 7, 2017

agramfort commented Jun 7, 2017

amueller Jun 20, 2017

amueller Jun 20, 2017

		@@ -131,9 +161,7 @@
		'CSCO': 'Cisco',
		'TXN': 'Texas instruments',

[MRG] Updated plot_stock_market.py to use Google Finance #9010

[MRG] Updated plot_stock_market.py to use Google Finance #9010

Conversation

superbobry commented Jun 6, 2017

Reference Issue

What does this implement/fix? Explain your changes.

Any other comments?

lesteve commented Jun 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superbobry Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

lesteve commented Jun 6, 2017

lesteve commented Jun 6, 2017

lesteve commented Jun 6, 2017

superbobry commented Jun 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vene commented Jun 6, 2017

lesteve commented Jun 6, 2017 • edited Loading

lesteve commented Jun 6, 2017

vene commented Jun 6, 2017

jnothman commented Jun 7, 2017

agramfort commented Jun 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superbobry Jun 6, 2017 •

edited

Loading

lesteve commented Jun 6, 2017 •

edited

Loading