FIX return proper instance class in displays classmethod

[johncant]

At the moment, if you try this:

class Subclass(RocCurveDisplay):
    pass

type(Subclass.from_predictions(....))

You would get RocCurveDisplay. If users want to reuse/override functionality, like I do, and invoke by calling these named constructors, this needs to return Subclass.

Easily fixed by instantiating cls in the named constructor methods instead of i.e. RocCurveDisplay.

[github-actions]

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

_{Generated for commit: 282c2d6. Link to the linter CI: here}

[glemaitre]

Yep it makes a lot of sense. I think that we can considered it as a bug.
Could you modify the other displays that suffer from the same pattern.
From what I see, the following will need to be modified as well.

• CalibrationDisplay
• PartialDependenceDisplay
• DecisionBoundaryDisplay

I think that we need an entry in the changelog as a FIX.
We also need a non-regression tests for these displays. We could for instance tests to subclassing is working as expected.

[johncant]

Hi - I still plan to do this

[glemaitre]

Cool. Thanks @johncant I am assigning the issue to you.
Do not hesitate to ping my GitHub handle when you submit the pull request.

[glemaitre]

Ups, this is actually already a pull-request :). I will be notified when you are going to make changes, so all good.

[johncant]

#Classes to fix (from it grep 'class [A-Za-z0-9]\+Display'):

Class	Named constructors need fixing	Now returns subclass instance	Non-regression test
CalibrationDisplay
DetCurveDisplay
PrecisionRecallDisplay
RocCurveDisplay
DecisionBoundaryDisplay:
PartialDependenceDisplay:
ConfusionMatrixDisplay
LearningCurveDisplay
ValidationCurveDisplay
PredictionErrorDisplay

I'll update this comment as I go

[johncant]

@glemaitre - this is what I've done:

• Fixed all *Display class named constructors to return the correct type
• Added non-regression tests for all the *Display classes, even those that didn't need fixing
• Updated the changelog

PR is now ready!

[glemaitre]

Thanks. I'm going to review it.

[glemaitre]

LGTM. If we make it before the release, I think we can include it in 1.4.0. Otherwise we will have to move it for 1.4.1.

[glemaitre]

I might have introduce a linter issue. You can run black as specified by the bot to solve it.

[johncant]

Hi @glemaitre - I've accepted all your suggestions and fixed the linter. LGTM!

[jeremiedbb]

LGTM. I'm +1 to include it in 1.4.0. @glemaitre you want to take another look ?

[glemaitre]

Fine on my side as well.