ping @ogrisel @madhuracj @thomasjpfan @adrinjalali @NicolasHug

😞 Yes, the indexing seems to be quite off. I am relieved this was found out before the feature was released. Thanks, @glemaitre for the quick fix.

I am relieved this was found out before the feature was released

Don't worry we already released bugs in the past :)

I find the code quite complex to review. I am not even sure what was the actual indexing bug to look for. Could you please describe it in two words? Would it happen in the "both" case? I am not sure I can see the difference visually on the rendered HTML example.

Our plotting was bug resilient :)
The reason is that we are generating the plot and storing the line into self.lines_[...] and it is this indexing which is wrong.
So the element in self.lines_ are not properly indexed but we are not using them to plot. However, if one want to use this attribute for some reason, it is wrongly indexed. A small example to show the bug:

    from sklearn.datasets import make_classification
    from sklearn.linear_model import LogisticRegression
    from sklearn.inspection import plot_partial_dependence
    X, y = make_classification()
    model = LogisticRegression().fit(X, y)
    disp = plot_partial_dependence(
        model, X, features=[0, 1, 2, 3], kind="both",
        subsample=3
    )

The axis will be organized on a grid of (2, 3)

disp.axes_.shape
(2, 3)

You can access the line of a plot with disp.lines_[0, 0] which is the index of the sub-axis. On our case, we have 4 lines (3 ICE and one mean.

disp.lines_[0, 1].shape
(4,)

The array disp.lines_ was pre-allocated with None. So now we can see the error of indexing:

disp.lines_[0, 1]
[<matplotlib.lines.Line2D object at 0x7fc12c0143a0> None
 <matplotlib.lines.Line2D object at 0x7fc12bff69a0> None]

So we have 2 values at None because we did not place the line at the right place.

My non-regression test check that there are no None values in this array.

I think that we should use np.ravel_multi_index to simplify the indexing. It will make things much more readable.

I refactor the code with a 1-way private function calling the ICE and PDP plotting function and a 2-way plotting function.
We have some slight redundancy to decorate the axis but I think that the code will be more readable as you suggested @ogrisel

I still have to document and maybe add a couple of comments to clarify the indexing or some parameters.

@ogrisel The code is ready for another review. A bit less easy to review but I think the refactoring makes it easier to go through the code, hopefully :).

ping @thomasjpfan I think that it should be ready now.

pinging @NicolasHug and @adrinjalali

@thomasjpfan no need for the what's new because we did not release it yet.
So I am merging.