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

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

Merged
merged 4 commits into from Jun 7, 2017

Conversation

Projects
None yet
6 participants
@superbobry
Contributor

superbobry commented Jun 6, 2017

Reference Issue

Fixes #8899.

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.

superbobry added some commits Jun 6, 2017

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

This comment has been minimized.

Member

lesteve commented Jun 6, 2017

Nice thanks for picking this up!

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

This comment has been minimized.

@lesteve

lesteve Jun 6, 2017

Member

I would not print the url.

This comment has been minimized.

@superbobry

superbobry Jun 6, 2017

Contributor

Yep, this is leftover debugging code. Will remove.

doc/conf.py Outdated
@@ -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': []

This comment has been minimized.

@lesteve

lesteve Jun 6, 2017

Member

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

This comment has been minimized.

@superbobry

superbobry Jun 6, 2017

Contributor

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

@lesteve

This comment has been minimized.

Member

lesteve commented Jun 6, 2017

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

from stable doc:

this PR (run locally on my laptop):
figure_1

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

@lesteve

This comment has been minimized.

Member

lesteve commented Jun 6, 2017

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

@lesteve

This comment has been minimized.

Member

lesteve commented Jun 6, 2017

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

@superbobry

This comment has been minimized.

Contributor

superbobry commented Jun 6, 2017

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.

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

This comment has been minimized.

@vene

vene Jun 6, 2017

Member

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".

This comment has been minimized.

@superbobry

superbobry Jun 6, 2017

Contributor

Done.

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

This comment has been minimized.

@vene

vene Jun 6, 2017

Member

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?

This comment has been minimized.

@lesteve

lesteve Jun 6, 2017

Member

close_prices/open_prices seems fine with me.

This comment has been minimized.

@superbobry

superbobry Jun 6, 2017

Contributor

Fixed.

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

This comment has been minimized.

@vene

vene Jun 6, 2017

Member

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.

This comment has been minimized.

@superbobry

superbobry Jun 6, 2017

Contributor

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

@vene

This comment has been minimized.

Member

vene commented Jun 6, 2017

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

This comment has been minimized.

Member

lesteve commented Jun 6, 2017

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

This comment has been minimized.

Member

lesteve commented Jun 6, 2017

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

This comment has been minimized.

Member

vene commented Jun 6, 2017

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

@jnothman

This comment has been minimized.

Member

jnothman commented Jun 7, 2017

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

@agramfort

This comment has been minimized.

Member

agramfort commented Jun 7, 2017

Circle is happy

thx @superbobry

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

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

4 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing e2d011d...c1d1541
Details
codecov/project 95.92% (+<.01%) compared to e2d011d
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

Sundrique added a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017

[MRG] Updated plot_stock_market.py to use Google Finance (scikit-lear…
…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

jakirkham added a commit to jakirkham/scikit-learn that referenced this pull request Jun 15, 2017

[MRG] Updated plot_stock_market.py to use Google Finance (scikit-lear…
…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

amueller added a commit that referenced this pull request Jun 19, 2017

Backport NumPy 1.13.0 fixes to 0.18.X (#9137)
* 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)
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])

This comment has been minimized.

@amueller

amueller Jun 20, 2017

Member

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

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

This comment has been minimized.

@amueller

amueller Jun 20, 2017

Member

this doesn't work in python2.7

dmohns added a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017

[MRG] Updated plot_stock_market.py to use Google Finance (scikit-lear…
…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

dmohns added a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017

[MRG] Updated plot_stock_market.py to use Google Finance (scikit-lear…
…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

NelleV added a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017

[MRG] Updated plot_stock_market.py to use Google Finance (scikit-lear…
…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

paulha added a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017

[MRG] Updated plot_stock_market.py to use Google Finance (scikit-lear…
…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

AishwaryaRK added a commit to AishwaryaRK/scikit-learn that referenced this pull request Aug 29, 2017

[MRG] Updated plot_stock_market.py to use Google Finance (scikit-lear…
…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

maskani-moh added a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017

[MRG] Updated plot_stock_market.py to use Google Finance (scikit-lear…
…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

jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017

[MRG] Updated plot_stock_market.py to use Google Finance (scikit-lear…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment