Skip to content
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

Add method mpf.kwarg_help() to print kwargs information. #416

Merged
merged 15 commits into from
Jan 11, 2022
Merged

Add method mpf.kwarg_help() to print kwargs information. #416

merged 15 commits into from
Jan 11, 2022

Conversation

lfenzo
Copy link
Contributor

@lfenzo lfenzo commented Jul 16, 2021

Added descriptions fields in the kwargs dicts for plotting. Only a few of the kwargs have descriptions, however the feature of displaying the kwargs with descriptions is already available with the function _display_formetted_kwargs_table() and other descriptions can be added. I wasn't sure where to place this feature without changing the API for plotting so I left it untouched, but I think this could be added either with a kwarg for the ´plot´ method or with a dedicated method,

@DanielGoldfarb
Copy link
Collaborator

DanielGoldfarb commented Jul 16, 2021

@lfenzo Thank you. I have been thinking about doing this for months and just never got around to it. This is amazing that you thought to do it!

I don't have time right now to look it over in detail. I would like to make a few comments and ask for a few changes:

  1. If you run pytest from the mplfinance directory, you will see that some of the tests fail for not having 3 keys in the valid kwarg dictionary. This is because there are actually many valid kwarg dicts throughout the code. All of these need to be changed to include the description key:
dino@DINO:~/code/mplfinance/src/mplfinance$ egrep -i 'def .*valid.*kwarg' *.py
_arg_validators.py:def _validate_vkwargs_dict(vkwargs):
_styles.py:def _valid_make_mpf_style_kwargs():
_styles.py:def _valid_make_marketcolors_kwargs():
_utils.py:def _valid_renko_kwargs():
_utils.py:def _valid_pnf_kwargs():
_utils.py:def _valid_lines_kwargs():
_widths.py:def _valid_scale_width_kwargs():
_widths.py:def _valid_update_width_kwargs():
plotting.py:def _valid_plot_kwargs():
plotting.py:def _valid_addplot_kwargs():
  1. I am not sure how I feel about alphabetizing the valid kwargs in the code. It can make the code more difficult to maintain, and to some extent I already had the kwargs organized somewhat by functionality. We can always alphabetize the dicts at the time of printing it out. The ones that you have already alphabetized, you may keep alphabetized for now. But please leave the others as they are when adding the description key.

  2. I was thinking of make a public function, maybe call it kwargs() or kwarg_help(). The user could pass in a function name, and get the kwargs for that function, for example:

    • mpf.kwarg_help('plot') to print the kwarg list for mpf.plot()
    • mpf.kwarg_help('make_addplot') to print the kwarg list for mpf.make_addplot()
      something like that. I haven't thought it through completely. I am open to suggestions. Also, I was thinking a user could give a specific kwarg to get the description for one kwarg: mpf.kwarg_help('make_addplot','alpha') to get the description for the alpha kwarg of mpf.make_addplot().

Please let me know your thought on the above. And again, thank you so much for contributing!
All the best. --Daniel

P.S. Meanwhile, I need to try to figure out why pytest did not automatically run when you submitted the PR. It is supposed to run automatically, and it was working fine about a month ago, but for some reason it did not run this time.

@lfenzo
Copy link
Contributor Author

lfenzo commented Jul 19, 2021

@DanielGoldfarb thanks for reviewing so quickly!

  1. I wasn't aware of the other valid kwargs in the code base, thanks for pointing out all of them for me. All of them have been changed to include descriptions. Unfortunately, since I knew only a tiny fraction of the kwargs, most of the descriptions are empty strings. For the tests I ran pytest tests/ resulting in:
============================= test session starts ==============================
platform linux -- Python 3.9.5, pytest-6.2.4, py-1.10.0, pluggy-0.13.1
rootdir: /var/home/enzo/Documents/projetos/mplfinance, configfile: tox.ini
collected 41 items

tests/test_addplot.py FF....F....F                                       [ 29%]
tests/test_alines.py F                                                   [ 31%]
tests/test_exceptions.py .......                                         [ 48%]
tests/test_hlines.py FF                                                  [ 53%]
tests/test_pnf.py .....                                                  [ 65%]
tests/test_renko.py .....                                                [ 78%]
tests/test_vlines.py FF.                                                 [ 85%]
tests/original_flavor/test_date_demo1.py .                               [ 87%]
tests/original_flavor/test_date_demo2.py .                               [ 90%]
tests/original_flavor/test_finance_demo.py .                             [ 92%]
tests/original_flavor/test_finance_work2.py .                            [ 95%]
tests/original_flavor/test_longshort.py .                                [ 97%]
tests/original_flavor/test_plot_day_summary_oclh_demo.py .               [100%]

...

=========================== short test summary info ============================
FAILED tests/test_addplot.py::test_addplot01 - AssertionError: assert 'Error:...
FAILED tests/test_addplot.py::test_addplot02 - AssertionError: assert 'Error:...
FAILED tests/test_addplot.py::test_addplot07 - AssertionError: assert 'Error:...
FAILED tests/test_addplot.py::test_addplot12 - AssertionError: assert 'Error:...
FAILED tests/test_alines.py::test_alines01 - AssertionError: assert 'Error: I...
FAILED tests/test_hlines.py::test_hlines01 - AssertionError: assert 'Error: I...
FAILED tests/test_hlines.py::test_hlines02 - AssertionError: assert 'Error: I...
FAILED tests/test_vlines.py::test_vlines01 - AssertionError: assert 'Error: I...
FAILED tests/test_vlines.py::test_vlines02 - AssertionError: assert 'Error: I...
======================== 9 failed, 32 passed in 12.77s =========================

All the errors above mention AssertionError: assert 'Error: Image files did not match.\n (...), so just to make sure that nothing was broken because of the addition of the descriptions I cloned matplotlib/mplfinance and ran pytest, which resulted in the same errors.

  1. I thought that sorting the kwargs alphabetically would make the code easier to maintain because it would allow maintainers to know exactly where a given kwarg should be (or if it even exists) in case of a relatively large kwarg dict like in _valid_plot_args(). But if they are organized according to their functionality, then for me it's ok.

  2. I think it's an interesting idea! As a user, I think I'd rather have something like mpf.kwarg_help(func: str, kwargs = None) in which kwargs can be either a single string or an iterable of strings (or even regular expressions) referring to kwargs of func (in this case Nonemeans "all kwargs"). This would allow the creation of "dynamic" kwarg description tables, depending on what the user wants.

Thank you for reviewing!
Best regards!

Enzo.

@DanielGoldfarb DanielGoldfarb changed the title Descriptions in valid kwargs table and method to print all kwargs information Add method mpf.kwarg_help() to print kwargs information. Jan 11, 2022
@DanielGoldfarb DanielGoldfarb merged commit bde1082 into matplotlib:master Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants