search

reiinakano / scikit-plot

An intuitive library to add plotting functionality to scikit-learn objects.
MIT License
2.43k stars 285 forks source link

#83 plot one class only for plot_precision_recall_curve #84

Closed lugq1990 closed 6 years ago

lugq1990 commented 6 years ago

83 plot one class only for plot_precision_recall_curve

reiinakano commented 6 years ago

Hi @lugq1990, first, thanks a lot for both your PRs. Sorry I've been very slow to review, I've been very busy the past three months. Anyway, let me drop a few comments on this PR first as this is pretty important to me.

I've long thought about redoing the API of curves so that users can select individual classes to plot, not the current way of just being able to select each_class.

Here's my idea of how this should look.

First, we need to deprecate curves.

We then add in plot_micro (boolean default true), plot_macro (boolean default true), and classes_to_plot that must take a list-like array of numbers determining which classes to plot. The default value of classes_to_plot must be None, which signifies that all curves are to be plotted.

This will take a bit of work, especially properly deprecating curves. Let me know if you still want to take it on.

lugq1990 commented 6 years ago

@reiinakano Hello, during the weekend I have deprecated the 'curves', instead as you have said using the plot_micro (boolean default true), plot_macro (boolean default true), and classes_to_plot that must take a list-like array of numbers determining which classes to plot. I will make the commit. Please check it during your free time. The result is as followed: 2018-02-26_142931

lugq1990 commented 6 years ago

@reiinakano I have made a new pull, but because the master branch test is still using the curves parameters to check, so it is verified fail. I have also changed the test file. Please check it.

reiinakano commented 6 years ago

Deprecation does not mean removing the argument immediately. You should detect whether a user is explicitly using curves, and emit a deprecation warning. But it should still work the same as before. Like I said, it's a bit tricky.

lugq1990 commented 6 years ago

@reiinakano It is really a little tricky. I have a idea but may not the best. We also add three parameters:plot_macro, plot_micor and classes_to_plot, keep the curves parameter at the same time. If user want to plot the each_class, we give the user a warning tell him that the parameter is deprecated, but we will still plot the precision_recall_curve and plot the micro and macro default. The added parameters plot_micro and plot_macro give the user control whether plot them or not, the classes_to_plot give the user control to plot the classes for the classes needed to be plot. As you have said : keep the plot_micro and plot_macro be True default. Is that OK?

reiinakano commented 6 years ago

Sorry for the late reply, yes, that sounds good

lugq1990 commented 6 years ago

It's Ok. I will make it as above as soon as possible.

lugq1990 commented 6 years ago

@reiinakano I have made it as we have discussed. If user choose the 'curves' want to plot the each_class, we just give a warning but also plot all classes for user.

reiinakano commented 6 years ago

Hi @lugq1990, I know it's been months, I'm really sorry but I finally got some free time to sit here and work on scikit-plot. Anyway, I made a few changes myself but I merged it in after that. Thanks a lot for your PR and again, really sorry it took this long to review!