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

Adds style related dictionaries to plot_decision_regions #342

Merged
merged 6 commits into from
Mar 15, 2018

Conversation

jrbourbeau
Copy link
Contributor

@jrbourbeau jrbourbeau commented Mar 13, 2018

Description

This PR adds parameters to plot_decision_regions that can be used to customize the plotting style used (e.g. size of scatter plot markers, alpha value of decision regions, etc.)

Related issues or pull requests

Fixes #340

Pull Request requirements

  • Added appropriate unit test functions in the ./mlxtend/*/tests directories
  • Ran nosetests ./mlxtend -sv and make sure that all unit tests pass
  • Checked the test coverage by running nosetests ./mlxtend --with-coverage
  • Checked for style issues by running flake8 ./mlxtend
  • Added a note about the modification or contribution to the ./docs/sources/CHANGELOG.md file
  • Modify documentation in the appropriate location under mlxtend/docs/sources/ (optional)
  • Checked that the Travis-CI build passed at https://travis-ci.org/rasbt/mlxtend

@jrbourbeau
Copy link
Contributor Author

jrbourbeau commented Mar 13, 2018

Additional TODO:

  • Make user can't override markers and colors parameters in plot_decision_regions with scatter_kwargs, etc.

@coveralls
Copy link

coveralls commented Mar 13, 2018

Coverage Status

Coverage increased (+0.2%) to 91.727% when pulling 3990f22 on jrbourbeau:add_decision_regions_kwargs into 4f148c4 on rasbt:master.


return ax


def format_plotting_kwargs(default_kwargs=None, user_kwargs=None,
Copy link
Owner

@rasbt rasbt Mar 14, 2018

Choose a reason for hiding this comment

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

That's a nice function dealing with the issues of protected keys! One little thing that comes to mind is that the way I wrote the API parser for the documentation, this would show up in the plotting API docs as well. You could make this function private by prefixing it with an underscore (_format_plotting_kwargs), but I think this function can come in very handy in other functions (esp. plotting functions) as well in future. So, it would be nice if you could move it to the mlxtend.utils :)

Copy link
Owner

Choose a reason for hiding this comment

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

I don't have a better name at hand, but under a more general name like "format_kwarg_dictionaries" or so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me! I have a couple of quick questions:

  1. Should this function go into its own module in utils, maybe plotting.py, or into one of the existing modules like checking.py?

  2. Should I also add the leading underscore to make this function private in utils (i.e. _format_kwarg_dictionaries vs. format_kwarg_dictionaries)? I'd imagine this function will only be used for internal stuff and not directly by users, so the leading underscore would make sense.

Thanks!

Copy link
Owner

Choose a reason for hiding this comment

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

Good points! I think we could put it into checking, because it's somewhat related to checking kwargs. I wouldn't suggest making it private, because it's generally useful (how I think about private things is that these are more like helper functions that are not useful outside the scope of a particular function and more like a side effect of refactoring, whereas the public ones are more broadly useful).

@jrbourbeau
Copy link
Contributor Author

I can also add a plot_decision_regions user guide example to illustrate how to use the style kwargs

@rasbt
Copy link
Owner

rasbt commented Mar 14, 2018

I can also add a plot_decision_regions user guide example to illustrate how to use the style kwargs

Thanks, that would be super nice :)

@jrbourbeau
Copy link
Contributor Author

Unless you have any other comments, I think this PR should be good to go!

@rasbt
Copy link
Owner

rasbt commented Mar 14, 2018

Based on the code, this looks great, thanks so much! I will take a look at the notebook as well and give it a try, but it looks perfect so far!

@rasbt
Copy link
Owner

rasbt commented Mar 15, 2018

Looks and works perfectly, thanks for the great contribution!

@rasbt rasbt merged commit eef3bb9 into rasbt:master Mar 15, 2018
@jrbourbeau jrbourbeau deleted the add_decision_regions_kwargs branch March 15, 2018 16:33
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.

None yet

3 participants