[MRG] Expose interpolation thresholds as public fitted attribute of IsotonicRegression

[kishimoto-banana]

I want to reconstruct fitted stepwise interpolation function of isotonic regression in other programming languages (Scala, Go, ...).

Therefore, I want to get interpolation points (input to function f_).

However, interpolation points (_necessary_X_, _necessary_y_) are internal variable of IsotonicRegression class according to PEP8 (leading underscores).

This PR changes interpolation points to attributes of IsotonicRegression class.

[agramfort]

why exposing more public attribute helps you in other languages?

[kishimoto-banana]

I want to save interpolation points (_necessary_X_, _necessary_y_) to files and read those files in other langages in order to reconsruct fitted stepwise interpolation function.

But because _necessary_X_ and _necessary_y_ are internal variable of IsotonicRegression class, getting them seems strange.

[agramfort]

as you will need to serialize the object manually I would suggest you deal with it in your code. These private attributes are not guaranteed to change but rather unlikely. If you want you can add a test that will break if we change these.

[ogrisel]

While I agree that in general we should not expose every thing as public attribute just for the purpose of making it easier for people to write model exporters, I am not opposed to exposing those attributes for this particular case, for model inspection purpose. There are only two attributes, this class has no other fitted attributes and those 2 arrays could be meaningfully plotted to graphically explain what the model does.

Actually this example (examples/plot_isotonic_regression.py) would really benefit to be updated to add a side plot of the X_threshods_ vs y_thresholds_ for education purpose.

But if we expose new public attributes we need more tests for the presence of those attributes and their values (e.g. that both arrays have the same length, that there values are increasing, in the range of the training data and that the length is not larger than the training set). This could be a good opportunity to check that the de-duplication code works as expected on some toy training sets with just a few well chosen samples that would illustrate some extreme cases.

[agramfort]

I am ok with that if we make it part of educational material in an example. +1 to rename as suggested too

…

[kishimoto-banana]

Thank you for following.
I will rename and add explanation for X_thresholds_ and y_thresholds_.

[ogrisel]

@kishimoto-banana are you also interested in updating the example as described in #16289 (review)? or do you want someone else to take over this PR to finish that part?

[kishimoto-banana]

@ogrisel Thank you for your comments.
I hope to take over updating the example to someone else.

[ogrisel]

LGTM :)

[ogrisel]

I cannot access the codecov report at the moment. I get HTTP error 500 whenever I click on the reports.

[ogrisel]

codecov was broken when uploading the coverage data of some of the jobs:

https://dev.azure.com/scikit-learn/scikit-learn/_build/results?buildId=13126&view=logs&j=78a0bf4f-79e5-5387-94ec-13e67d216d6e&t=80793e4d-ec39-5212-d625-56647bcbed6e&l=43

Hence the decrease in coverage.

[ogrisel]

Alright, code coverage is now happy.

[ogrisel]

Here is the plot of the updated example:

[ogrisel]

Thanks @thomasjpfan for fixing the PR. Would you like to review it?

[NicolasHug]

Thanks @kishimoto-banana , looks good.

In the prediction function plot it'd be good to also plot the extrapolation lines, i.e. beyond X.min() and X.max(). If it makes the code less clear, a simple sentence could be enough:

Beyond the limits of the training data (i.e. before X.min() and after X.max()), the prediction corresponds to pred(X.min()) and pred(X.max()) respectively.

@ogrisel I'm not quite sure about keeping backward compat here?

[ogrisel]

I believe I have addressed all the pending review comments @NicolasHug @thomasjpfan.

[ogrisel]

Here is the new example figure with clipping based extrapolation:

[NicolasHug]

Thanks both, some minor comments but LGTM anyway