Script additions for 'ema' plotting by andrewrgarcia · Pull Request #563 · matplotlib/mplfinance (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation17 Commits9 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Relates to Issue #506

What was done

Added a new _plot_ema() function to the plotting.py file similar to _plot_mav where the main difference is in line 1176 of the requested script.

Also added an ema key to the _valid_plot_kwargs() function in plotting.py which also uses the _mav_validator because ema should have the same datatype as mav.

andrewrgarcia added 2 commits

October 19, 2022 14:40

…a has the same data-types as mav

simple protocol for unit tests

I forgot to mention. I was wondering if someone may help me by suggesting a concise protocol for running unit tests on this repository (for example, run python setup.py tests, set up an init.py file, something like that). I tried doing so, but the tests were run with my installed version of mpf and not with the changes I was making. It would be much appreciated, thanks in advance 🙏

simple protocol for unit tests

@andrewrgarcia
Andrew, Thanks! I should probably document the following.
I develop and test is as follows:

1. cd into the main mplfinance directory (the one that contains setup.py)
2. run pip install -e ./

The ./ will install from your local cloned version, and -e makes it "editable" meaning any changes you make to the code will be immediately reflected in the installed version. (However if you are running jupyter, or something like that, you will need to restart the kernal (or re-import the package) to pick up the changes).

I have found the above to be the simplest way to develop the package. I make my changes to the code, and then typically run my initial tests in a copy of one of the tutorial notebooks (or write a separate script to test).

If other people are working on the same machine as you, and using mplfinance, and you don't want to disturb them with your development version, just do the above inside a python virtual environment. But if you are alone on the machine I personally find it simplest to just install the dev version for the whole machine. You can always re-install the production version later.

Once you have done the above, in addition to whatever development and tests you want to run in scripts or notebooks, you can also run mplfinance's battery of regression tests by, again, cd'ing into the main mplfinance directory, and simply running pytest. This will run all of the tests inside the mplfinance/tests directory.
To run just a particular test file, you can run pytest tests/<test_file.py> (for example pytest tests/test_kwarg_help.py).

@andrewrgarcia

Andrew- The code looks great so far. Of course you will have to add calls to _plot_ema() in various places throughout plotting.py; probably the best strategy is to add a call to _plot_ema() near every place where there is already a call to _plot_mav()

I believe the way _plot_mav() and _plot_ema() are written, both can always be called (as _plot_mav() is called now), and they will do work or return nothing depending on whether the user specified mav= and/or ema= kwargs.

The mildly tricky part will be to get both mav and ema appropriately into the return_calculated_values dict. If you are not familiar with this kwarg, take a look at the documentation here and see the code here.

Thank you so much for contributing. All the best. --Daniel

simple protocol for unit tests

@andrewrgarcia Andrew, Thanks! I should probably document the following. I develop and test is as follows:

@DanielGoldfarb Thank you Dr. Goldfarb. I found your explanation quite practical and it works very well! I suggest you add your explanation as a subsection to the Coding Standards section in the CONTRIBUTING.md document as I think it can help others run tests as well.

I suggest you add your explanation as a subsection to the Coding Standards section in the CONTRIBUTING.md document ...

Those were my thoughts exactly. Thanks. And congrats on finishing your PhD this year.

@andrewrgarcia

Andrew- The code looks great so far. Of course you will have to add calls to _plot_ema() in various places throughout plotting.py; probably the best strategy is to add a call to _plot_ema() near every place where there is already a call to _plot_mav()

@DanielGoldfarb Thank you for pointing me to those resources. That's a good point; as _plot_mav() shares many similarities with a potential _plot_ema() , I am trying to figure out how to add the additional functionality by making as little changes to the code as possible.

I suggest you add your explanation as a subsection to the Coding Standards section in the CONTRIBUTING.md document ...

Those were my thoughts exactly. Thanks. And congrats on finishing your PhD this year.

@DanielGoldfarb Thanks so much.

@DanielGoldfarb I just made the changes to make the new ema option work and run a test which compared the built-in mav and ema options against calculated mav and ema in tests/test_ema.py (corresponding image in tests/test_images/test_ema.png).

@andrewrgarcia - Thanks for writing the test. Looks good. I will be running a few tests myself to feel sure on my end. Meanwhile, it appears that perhaps you used a tool to "standardize" the code. As noted in the mplfinance coding standards section of the contributing page "If you work on a pre-existing code file, please try to more-or-less emulate the style that already exists in that file."

I am definitely open to discussing whether we should make all mplfinance code compliant with a particular style (PEP 8). If we decide to make such a change, my experience has shown that its best to merge such changes (which in theory should have zero effect on functionality) as their own separate Pull Request. Meanwhile, please reverse the style changes so that this PR contains only changes directly related to adding functionality for the Exponential Moving Average. If you don't have the time, or if for any other reason you can't revese only the style changes, then please let me know and I will take care of it within this PR.

Thanks. Much appreciated. And thanks again for your interest in, and contributions to, mplfinance.

andrewrgarcia added 2 commits

October 24, 2022 15:41

…line"

This reverts commit 95f0e33.

@andrewrgarcia - Thanks for writing the test. Looks good. I will be running a few tests myself to feel sure on my end. Meanwhile, it appears that perhaps you used a tool to "standardize" the code. As noted in the mplfinance coding standards section of the contributing page "If you work on a pre-existing code file, please try to more-or-less emulate the style that already exists in that file."

I am definitely open to discussing whether we should make all mplfinance code compliant with a particular style (PEP 8). If we decide to make such a change, my experience has shown that its best to merge such changes (which in theory should have zero effect on functionality) as their own separate Pull Request. Meanwhile, please reverse the style changes so that this PR contains only changes directly related to adding functionality for the Exponential Moving Average. If you don't have the time, or if for any other reason you can't revese only the style changes, then please let me know and I will take care of it within this PR.

Thanks. Much appreciated. And thanks again for your interest in, and contributions to, mplfinance.

@DanielGoldfarb that was my mistake; it appears my VS Code editor was using a "Format on Save" option which was formatting the whole code. I just reverted that commit and made only the necessary changes to the exponential mav as suggested. On this new commit, I am also doing my best to comply with PEP 8 formatting as you can see on the new tests/test_ema.py file.

Thank you so much for your feedback and all your patience; it's helped me a lot. Let me know what you think of the new changes.

@andrewrgarcia -- Thanks! One other thing I was thinking but forgot to mention. In tests/test_ema.py you access https://api.binance.com/api/v1/klines to get data. I would like to keep mplfinance independent of any data acquisition API's.

(As a side but related note, I have also tried to minimize the number of other packages mplfinance is dependent upon. Obviously it must depend on matplotlib and on pandas, followed by the dependences of those two packages. But beyond that I've strived not to include any additional dependencies).

All data used by examples and tests should be placed in the mplfinances/examples/data/ folder as a csv file. Presently all of the tests in the test folder use a single data set as input. Reading this data is done in file tests/conftest.py ("configuration [for] tests") which is a file that pytest runs at the beginning of every test. The data is then passed into each test via a pytest.fixture (the data appears as a function passed as an argument to the test method).

That said, while it may keep things consistent for test_ema.py to get data in the same way as the other test files, quite possibly sooner or later we will have other tests that require different data as input. Therefore I have no problem if you want to use a different data file as input. Your call. Whatever you want to do is fine. I only ask that you remove access to the binance api from the test itself.

If you want to use the binance data, I would suggest writing a separate little script to go to the binance api, and then save the data in a csv file (using pandas DataFrame.to_csv()). If you want, you can save the separate little script in the mplfinance/examples/scratch_pad/ directory. That is a directory that I have reserved in the repository for various types of little scripts, tests, experimentation, etc. that are not officially part of the package installation, but which we may want to save "for posterity." (In fact, on various occasions I have gone back to that directory to remind myself of something that I experimented with in the past). Please let me know if you have any questions about this. And once again, thank you for contribution!

@andrewrgarcia -- Thanks! One other thing I was thinking but forgot to mention. In tests/test_ema.py you access https://api.binance.com/api/v1/klines to get data. I would like to keep mplfinance independent of any data acquisition API's.

(As a side but related note, I have also tried to minimize the number of other packages mplfinance is dependent upon. Obviously it must depend on matplotlib and on pandas, followed by the dependences of those two packages. But beyond that I've strived not to include any additional dependencies).

All data used by examples and tests should be placed in the mplfinances/examples/data/ folder as a csv file. Presently all of the tests in the test folder use a single data set as input. Reading this data is done in file tests/conftest.py ("configuration [for] tests") which is a file that pytest runs at the beginning of every test. The data is then passed into each test via a pytest.fixture (the data appears as a function passed as an argument to the test method).

That said, while it may keep things consistent for test_ema.py to get data in the same way as the other test files, quite possibly sooner or later we will have other tests that require different data as input. Therefore I have no problem if you want to use a different data file as input. Your call. Whatever you want to do is fine. I only ask that you remove access to the binance api from the test itself.

If you want to use the binance data, I would suggest writing a separate little script to go to the binance api, and then save the data in a csv file (using pandas DataFrame.to_csv()). If you want, you can save the separate little script in the mplfinance/examples/scratch_pad/ directory. That is a directory that I have reserved in the repository for various types of little scripts, tests, experimentation, etc. that are not officially part of the package installation, but which we may want to save "for posterity." (In fact, on various occasions I have gone back to that directory to remind myself of something that I experimented with in the past). Please let me know if you have any questions about this. And once again, thank you for contribution!

@DanielGoldfarb got it. No problem, I will modify test_ema.py so that it calls one of the files already present in the mplfinances/examples/data/ folder and remove the https://api.binance.com/api/v1/klines API Binance call.

One other thing I just realize you perhaps did not realize about the tests:

Take a look, for example, at file test_alines.py (that's a simple one with only one test defined inside). When you run the test, it creates a png image of the plot in the test_images directory.

The first time ever that you run a test, you have to eye-ball that plot an make sure it looks good. After that you copy the image into the tests/reference_images directory.

Then the test itself is to recreate the plot image in the test_images directory, and compare it to the corresponding image in the reference_images directory. (See, for example, result = compare_images(rname,tname,tol=IMGCOMP_TOLERANCE) in test_alines.py). HTH.

andrewrgarcia added 2 commits

October 24, 2022 17:26

One other thing I just realize you perhaps did not realize about the tests:

Take a look, for example, at file test_alines.py (that's a simple one with only one test defined inside). When you run the test, it creates a png image of the plot in the test_images directory.

The first time ever that you run a test, you have to eye-ball that plot an make sure it looks good. After that you copy the image into the tests/reference_images directory.

Then the test itself is to recreate the plot image in the test_images directory, and compare it to the corresponding image in the reference_images directory. (See, for example, result = compare_images(rname,tname,tol=IMGCOMP_TOLERANCE) in test_alines.py). HTH.

@DanielGoldfarb I replaced the former example with one that makes a call to the .csv file for the historical price data of GOOG in mplfinances/examples/data/. Removed all former API call functions. I also applied the reference comparison algorithm as it was done in test_alines.py

@andrewrgarcia
Andrew, I am making a few changes to the code which I will soon push into this Pull Request. If you have the time, I'd like you please to code review them before I merge the PR into the production repository. The changes are as follows:

1. There is no need for you to call test_ema() at the bottom of test_ema.py. Package pytest automatically calls all functions (defined in the file) that begin test....
2. I added two more tests to test_ema.py to exercise a couple more use-cases. In the process, I generalized getting the data (so all three tests get the same data), and I add a few lines to clean up any old test_images.
3. I came to realized that both of us overlooked how the cycle of moving average colors was being used: two separate instances were being used, one for sma and one for ema, the result being that if someone chose to plot both sma and ema, then the sma and ema lines could end up having the same colors.
   To solve this problem, I modified the code slightly to place the cycle of colors into the config dict, so that _plot_mav() and _plot_ema() can share the same cycle object thereby making it unlikely that the sma and ema lines will have the same color. Let me know if you have any questions about this change to the code. I also, while I was at it, added kwarg mavcolors which is able to override the mavcolors that are part of the mpf style being used.

Let me know you see any issues with my changes. If not, then I will go ahead and merge the code.
Once again, thank you so much for contributing to mplfinance.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes are good.

@andrewrgarcia Andrew, I am making a few changes to the code which I will soon push into this Pull Request. If you have the time, I'd like you please to code review them before I merge the PR into the production repository. The changes are as follows:

1. There is no need for you to call `test_ema()` at the bottom of `test_ema.py`.   Package `pytest` automatically calls all functions (defined in the file) that begin `test...`.

2. I added two more tests to `test_ema.py` to exercise a couple more use-cases.  In the process, I generalized getting the data (so all three tests get the same data), and I add a few lines to clean up any old test_images.

3. I came to realized that both of us overlooked how the cycle of moving average colors was being used: two separate instances were being used, one for sma and one for ema, the result being that if someone chose to plot both sma and ema, then the sma and ema lines could end up having the same colors.
  To solve this problem, I modified the code slightly to place the cycle of colors into the `config` dict, so that `_plot_mav()` and `_plot_ema()` can share the same cycle object thereby making it unlikely that the sma and ema lines will have the same color.  Let me know if you have any questions about this change to the code.  I also, while I was at it, added kwarg `mavcolors` which is able to override the mavcolors that are part of the mpf style being used.

Let me know you see any issues with my changes. If not, then I will go ahead and merge the code. Once again, thank you so much for contributing to mplfinance.

@DanielGoldfarb The cycling for the simulaneous sma and ema use-case makes sense. On that same note, I also think the new mavcolors kwarg is a great implementation and a nice color customization feature of the moving average lines.

Thank you as well, these are useful additions to the library.

2 participants