finance ochl->ohlc #1920

Merged
merged 29 commits into from Aug 17, 2013

Projects

None yet

7 participants

@tacaswell
Owner

Implemented wrapper layer to merge in changes from PR #1783

@NelleV NelleV commented on an outdated diff Apr 22, 2013
lib/matplotlib/finance.py
@@ -26,6 +26,7 @@
from matplotlib.patches import Rectangle
from matplotlib.transforms import Affine2D
+from matplotlib import MatplotlibDeprecationWarning as mplDeprecation
NelleV
NelleV Apr 22, 2013 Contributor

it is best to import this from the cbook module.

@NelleV NelleV commented on an outdated diff Apr 22, 2013
lib/matplotlib/finance.py
+
+ warnings.warn("This function has been deprecated in 1.3 in favor"
+ "of `plot_day_summary_ochl`,"
+ "which maintains the natural argument order,"
+ "or `plot_day_summary_ohlc`,"
+ "which uses the open-high-low-close order."
+ "This function will be removed in 1.4", mplDeprecation)
+ return plot_day_summary_ohlc(ax, opens, highs, lows, closes, ticksize,
+ colorup, colordown)
+
+
+def plot_day_summary_ochl(ax, opens, closes, highs, lows, ticksize=4,
+ colorup='k', colordown='r',
+ ):
+
+ """
NelleV
NelleV Apr 22, 2013 Contributor

As much as possible, try using the new MEP10 conventions for the documentation. It corresponds to numpydoc's formatting.

More detail can be found here: https://github.com/matplotlib/matplotlib/wiki/Mep10

Owner

I will get to the documentation changes later.

@efiring efiring commented on an outdated diff May 11, 2013
lib/matplotlib/finance.py
@@ -486,46 +532,48 @@ def candlestick2(ax, opens, closes, highs, lows, width=4,
return value is lineCollection, barCollection
"""
- # note this code assumes if any value open, close, low, high is
+ # note this code assumes if any value open, low, high, close is
efiring
efiring May 11, 2013 Owner

Don't you want this comment to use the order open, high, low, close?

@efiring efiring and 1 other commented on an outdated diff May 11, 2013
lib/matplotlib/finance.py
@@ -391,70 +389,118 @@ def plot_day_summary2(ax, opens, closes, highs, lows, ticksize=4,
return value is a list of lines added
"""
- # note this code assumes if any value open, close, low, high is
+ warnings.warn("This function has been deprecated in 1.3 in favor"
+ "of `plot_day_summary_ochl`,"
+ "which maintains the natural argument order,"
efiring
efiring May 11, 2013 Owner

Isn't it confusing to call this the "natural argument order", when everything is being converted to the standard financial industry order (if I understand correctly)? Maybe just call it the "original matplotlib argument order"? Or is there some larger rationale or citation for calling it "natural"? (To me, as a non-financial person, it does seem quite natural--but since this is a function for financial people, what seems natural to me is completely irrelevant.)

tacaswell
tacaswell May 11, 2013 Owner

I'll take the editorial comment out.

@efiring efiring and 1 other commented on an outdated diff May 11, 2013
lib/matplotlib/finance.py
@@ -467,10 +513,10 @@ def plot_day_summary2(ax, opens, closes, highs, lows, ticksize=4,
return rangeCollection, openCollection, closeCollection
-def candlestick2(ax, opens, closes, highs, lows, width=4,
+def candlestick2(ax, opens, highs, lows, closes, width=4,
efiring
efiring May 11, 2013 Owner

Doesn't this one need the same sort of treatment to handle the change in argument order?

tacaswell
tacaswell May 11, 2013 Owner

I missed that change. Do we want to rename this to candlestick_ohlc?

Owner

Still haven't gotten to the documentation changes. I've been kinda crazy the last month or so.

@NelleV It looks like none of the functions in this file are the numpy style, should I do just the functions I touched, or would it be better to make a different PR with all of the documentation fixed?

Owner

@mdboom This is a API change and should probably go in 1.3 if it is going to go in.

Owner
mdboom commented May 13, 2013

@tacaswell: Thanks. It should get an entry in whats_new and api_changes and a test (probably doesn't need an image comparison test) and then I think it's good to go.

Owner

Going over this again last night I noticed some additional subtle api changes that I had missed before. The expected order of elements a nested sequence are also changed from [(d, open, close, high, low, ...), ...] -> [(d, open, high, low, close, ...), ...]. This affects the return values of the yahoo related functions and any of the functions that take quotes as an argument. Should all of these functions have *_ochl and *_ohlc versions created as well?

What sort of test should this get if not an image comparison? I don't think anything in this file is currently tested in anyway.

Owner

need to merge PR #2077 into this branch

(mostly a note to my self so I can find it again later)

Owner

Now that I have touched basically every function in this module, what tests should I add?

Should I smash most of these commits down to one?

@NelleV How should I deal with the very long URL on line 395?

Contributor
NelleV commented May 30, 2013

@NelleV https://github.com/NelleV How should I deal with the very long
URL on line 395?

It seem to be a string, hence you can split it as you would split a string:
urlname = ("blah" +
"blah")

or
urlname = ("blah"
"blah")

there has been discussion on python-dev on removing the latter, so I would
go with the splitting with the + sign.


Reply to this email directly or view it on GitHubhttps://github.com/matplotlib/matplotlib/pull/1920#issuecomment-18661681
.

Owner

@dmcdougall Now that things have calmed down a bit can this get some attention?

@NelleV NelleV commented on an outdated diff Jul 16, 2013
lib/matplotlib/finance.py
@@ -26,6 +29,7 @@
from matplotlib.patches import Rectangle
from matplotlib.transforms import Affine2D
+from cbook import mplDeprecation
NelleV
NelleV Jul 16, 2013 Contributor

Nitpick: can you please use explicit imports? from .cbook import mplDeprecation

NelleV
NelleV Jul 16, 2013 Contributor

(Or absolute imports)

@NelleV NelleV commented on an outdated diff Jul 16, 2013
lib/matplotlib/finance.py
+ where d is a floating poing representation of date,
+ as returned by date2num, and date is a python standard
+ library datetime.date instance.
+
+ The name of this kwarg is a historical artifact. Formerly,
+ True returned a cbook Bunch
+ holding 1-D ndarrays. The behavior of a numpy recarray is
+ very similar to the Bunch.
+
+ """
+ return _parse_yahoo_historical(fh, adjusted=adjusted, asobject=asobject,
+ ochl=True)
+
+
+def parse_yahoo_historical_ohlc(fh, adjusted=True, asobject=False):
+ """Parse the historical data in file handle fh from yahoo finance.
NelleV
NelleV Jul 16, 2013 Contributor

Can you please add a blank line after the docstring summary?

@NelleV NelleV commented on an outdated diff Jul 16, 2013
lib/matplotlib/finance.py
@@ -206,10 +422,136 @@ def fetch_historical_yahoo(ticker, date1, date2, cachename=None,dividends=False)
def quotes_historical_yahoo(ticker, date1, date2, asobject=False,
- adjusted=True, cachename=None):
+ adjusted=True, cachename=None):
+ """ Get historical data for ticker between date1 and date2.
+
+
+ This function has been deprecated in 1.4 in favor of
+ `quotes_yahoo_historical_ochl`, which maintains the original argument
+ order, or `quotes_yahoo_historical_ohlc`, which uses the
+ open-high-low-close order. This function will be removed in 1.5
+
+ See :func:`parse_yahoo_historical` for explanation of output formats
+ and the *asobject* and *adjusted* kwargs.
+
+ Ex:
NelleV
NelleV Jul 16, 2013 Contributor

This should probably be in an "examples" section.

@NelleV NelleV commented on the diff Jul 16, 2013
lib/matplotlib/finance.py
@@ -49,13 +66,119 @@
('volume', np.float),
('aclose', np.float)])
+_warn_str = ("This function has been deprecated in 1.4 in favor "
NelleV
NelleV Jul 16, 2013 Contributor

I'd rather not use this, and have the text duplicated in the deprecation warnings. It makes removing functions that are deprecated much easier.

Also, do we only wait one release before removing deprecated function? I think it should be two release, but maybe someone else can confirm.

WeatherGod
WeatherGod Jul 16, 2013 Member

IIRC, the standing rule is one release, however we do make exceptions for
some things that might have been more widely used.

tacaswell
tacaswell Jul 16, 2013 Owner

How strong is your 'rather not'? In principle I agree but this branch is a unicorn as it basically depreciates every function in this module.

mdboom
mdboom Jul 16, 2013 Owner

I think I agree with @tacaswell here: let's not duplicate this message N times.

Owner
efiring commented Jul 16, 2013

I don't want to interfere with all the work that has gone into trying to straighten out this module, but it raises a larger question: should such a specialized module even be part of the mpl core distribution? I think the answer is clearly "no". It should be in an external toolkit, or an example, or a cookbook, but it should not be accessible via "from matplotlib import ...".

@mdboom mdboom commented on an outdated diff Jul 16, 2013
doc/api/api_changes.rst
@@ -11,6 +11,13 @@ help figure out possible sources of the changes you are experiencing.
For new features that were added to matplotlib, please see
:ref:`whats-new`.
+Changes in 1.4.x
+================
+
+* In :module:`~matplotlib.finance`, almost all functions have beeen depreciated and
mdboom
mdboom Jul 16, 2013 Owner

depreciated -> deprecated

@mdboom mdboom commented on an outdated diff Jul 16, 2013
lib/matplotlib/finance.py
-def parse_yahoo_historical(fh, adjusted=True, asobject=False):
+
+def parse_yahoo_historical_ochl(fh, adjusted=True, asobject=False):
+ """Parse the historical data in file handle fh from yahoo finance.
+
+ Parameters
+ ----------
+
+ adjusted : `bool`
mdboom
mdboom Jul 16, 2013 Owner

For numpydoc format, don't use the backticks around types or :class:None etc... That all gets handles automatically.

Owner
mdboom commented Jul 16, 2013

In response to @efiring: I agree, this is highly specialized, and long term should probably be (at least) be moved to mpl_toolkits, or even better to its own independent project. But there are a lot of other things in matplotlib in that boat and I'd rather devise a plan for outward migration that works well for all such things. I don't think we should let that hold up this pull request which is valuable improvement in any event.

@tacaswell: Can you rebase so that the diff is clean again and we can go through for a (hopefully last) round of reviews?

Owner

Currently traveling, I will get to this over the weekend.

I have no protest to this getting pulled out.

Tom
On Jul 16, 2013 5:22 PM, "Michael Droettboom" notifications@github.com
wrote:

In response to @efiring https://github.com/efiring: I agree, this is
highly specialized, and long term should probably be (at least) be moved to
mpl_toolkits, or even better to its own independent project. But there
are a lot of other things in matplotlib in that boat and I'd rather devise
a plan for outward migration that works well for all such things. I don't
think we should let that hold up this pull request which is valuable
improvement in any event.

@tacaswell https://github.com/tacaswell: Can you rebase so that the
diff is clean again and we can go through for a (hopefully last) round of
reviews?


Reply to this email directly or view it on GitHubhttps://github.com/matplotlib/matplotlib/pull/1920#issuecomment-21074865
.

kramer650 and others added some commits Feb 25, 2013
@kramer650 @tacaswell kramer650 Update of finance.py to (O,H,L,C) instead of (O,C,H,L)
In the financial world, a bar-chart is commonly called an OHLC-chart or "Open-High-Low-Close chart". Also see http://en.wikipedia.org/wiki/Open-high-low-close_chart or simply google around.

To my surprise this library uses a sequence of OCHL instead of OHLC. Although it changes the API, I found it important to comply with financial industry standards. This makes it easier to implement this library in existing financial software and thus more likely to be used in the future.
2c4727a
@tacaswell @tacaswell tacaswell implemented the wrappers discussed in #1783.
Added deprecation warning to `plot_day_summary2` and returned call
signature to what it was.

Added alias `plot_day_summary_ochl` with same call signature as
`plot_day_summary2`.

renamed the modified function to `plot_day_summary_ohlc`.

The first two functions simply call the third with the arguments
re-ordered.
8745e3a
@tacaswell tacaswell import and pep8 changes f16dc47
@tacaswell @tacaswell tacaswell tweaked wording of documentation ffb2074
@tacaswell @tacaswell tacaswell wrapped candlestick2 to have new argument order 35d70d8
@tacaswell @tacaswell tacaswell minor documentation tweak 4ab6b95
@tacaswell @tacaswell tacaswell added depreciation warning + fixed error is function call argument
order
4ee6d2e
@tacaswell @tacaswell tacaswell converted all the docstrings numpydoc format 428333d
@tacaswell tacaswell wrapped and depreciated parse_yahoo_historical
fixed bug in original patch
e509cd2
@tacaswell tacaswell commented out unused line 9c5e1c3
@tacaswell tacaswell fixed pep8, updated depreciation messages because this missed 1.3 ca8845e
@tacaswell tacaswell fixed documentation 45eea6e
@tacaswell tacaswell wrapped and deprecated plot_day_summary
fixed naming on plot_day_summary2_*
10adeb2
@tacaswell tacaswell fixed documentation
wrapped candlestick
96b859f
@tacaswell tacaswell re-factored to actually raise depreciation warnings f4dcc2f
@tacaswell tacaswell wrapped and deprecated quotes_historical_yahoo c8fcc1a
@tacaswell tacaswell fixed naming bugs a92039b
@tacaswell tacaswell fixed a collection of stupid bugs (like not returning from
wrapper functions).
1def459
@tacaswell tacaswell added note to api_change.rst e4fab5d
@tacaswell tacaswell documentation corrections 469e267
@tacaswell tacaswell minor pep8 fix 007754c
@tacaswell @tacaswell tacaswell minor formatting changes to doc-strings
tweaked import
58d741f
@tacaswell tacaswell spelling + formatting fixes 62f4e19
Owner

I don't understand the errors coming out of travis, two of them look like AttributeErrors coming out of nose and the third is a font issue.

Owner
mdboom commented Jul 22, 2013

👍 from me. Thanks for all of your hard work on this!

Owner

The travis failures are unrelated, because this module has no test coverage.

Owner

As near as I can tell, the documentation for this module is not generated by sphinx.

I added it to the api list, but this seems at cross purposes to pulling this module out of matplotlib proper.

Owner
mdboom commented Aug 5, 2013

I think as long as it's in matplotlib, it should probably be in the documentation. Maybe just add a module-level docstring (which would also appear in the HTML docs), that says this module is deprecated?

@dmcdougall dmcdougall merged commit 17216bb into matplotlib:master Aug 17, 2013

1 check failed

default The Travis CI build failed
Details
@tacaswell tacaswell deleted the tacaswell:ochl_to_ohlc branch Sep 11, 2013
@tacaswell tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jan 10, 2015
@tacaswell tacaswell MNT : removed deprecated code in finance.py
Deprecated in #1920

merged to master in 17216bb
02baab7
@tacaswell tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jan 22, 2015
@tacaswell tacaswell MNT : removed deprecated code in finance.py
Deprecated in #1920

merged to master in 17216bb
48b8e6b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment