plot_decision_regions: Deprecated 'res' in favour of 'num'

[arteymix]

Might be a bit bold move, but the default on this basically yield good plots independently of the data range, so it's just better to warn for using res and introduce a new num parameter.

It also lookup the current figure to determine an optimal default (i.e. dpi * size).

[pep8speaks]

Hello @arteymix! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on December 23, 2017 at 05:59 Hours UTC

[rasbt]

Thanks for the PR. I like the idea of keeping but deprecating the "old" res parameter.
Just looking at the naming, res was probably not the ideal choice :P. Regarding the new num parameter, maybe we can have a more explicit parameter name. Maybe sth like num_points or grid_points or sth like that?

Currently, there seems to be one of the unit tests causing issues with the PR. Would be nice if you could take a look at it. Alternatively, I am also happy to help out with that (I think you would need to enable the "allow maintainer commits" option though

[arteymix]

I was thinking of having no option at all as it already yield the best graph for the current DPI.

The maintainer edit option is already checked if you want to fix the unit test.

[rasbt]

So, in other words, you were thinking of removing num as a parameter? I was playing around with your implementation, and I think that having "num" accepting integers etc is a bit dangerous. E.g., see the following examples:

So what I mean is that users, if not careful, could easily get misleading results.

I think the best way would be to remove num but keep the current default -- I really like your solution to this! Also, I would suggest changing the default of res to None. Then, res gets only used if a non-None value is provided. What do you think?

[arteymix]

Honestly, I'd remove it too but leave res do nothing for backward-compatibility. If user wants more resolution, then he should just set:

plt.figure(dpi=600)
plot_decision_regions(...)

[rasbt]

Good point. Actually, I would leave it for the sake of backward comp. but only let it do sth if it's not set to None (plus add the deprecation warning). Regarding

plt.figure(dpi=600)
plot_decision_regions(...)

that sounds good. Were you planning on updating the documentation for that?

[arteymix]

Yes yes! It's almost all good now. I also added anti-aliasing to make it look neat:

[rasbt]

wow, those plots look amazing! Thanks so much! I will be checking out your code now and fix the unit tests

[rasbt]

I think it should be good now! Once the ci-tests pass and you don't have further suggestions (or comments on the "cosmetic changes"), I think it should be good to merge! :)

[coveralls]

Coverage decreased (-0.03%) to 91.216% when pulling ef64a6a on arteymix:deprecate-res-in-plot-decision-regions into 1880299 on rasbt:master.

[arteymix]

I think it's good to merge!

There's still one thing I wanted to address: savefig does not re-render the decision frontier, so it will not look as good if the DPI is higher. The workaround is to render the figure with a good DPI and then save it.

[rasbt]

There's still one thing I wanted to address: savefig does not re-render the decision frontier, so it will not look as good if the DPI is higher

I think it's fine! Not sure how other people use this function, but I typically want to save the version that I generated & viewed :P. So, if you create the version you are happy with and then call plt.savefig() (before plt.show()), this should yield the "desired" plot, correct?!