Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistence between CalibrationDisplay and over Display #21027

Closed
glemaitre opened this issue Sep 13, 2021 · 9 comments · Fixed by #21031
Closed

Inconsistence between CalibrationDisplay and over Display #21027

glemaitre opened this issue Sep 13, 2021 · 9 comments · Fixed by #21031

Comments

@glemaitre
Copy link
Member

While working on #20999, I could notice that CalibrationDisplay has a name parameter while other displays have a estimator_name parameter. In #20999, I try to create a base class that manages, in the same manner, this parameter and it would be handy to have the same naming.

@thomasjpfan @lucyleeow Do you remember if there is a reason for choosing name instead of estimator_name in this display?
@adrinjalali If the change makes sense, I would submit a PR and it would be great to include before the release to avoid a deprecation cycle for this brand new display.

@thomasjpfan
Copy link
Member

thomasjpfan commented Sep 13, 2021

@thomasjpfan @lucyleeow Do you remember if there is a reason for choosing name instead of estimator_name in this display?

I think the initial API used "estimator_name", because I thought "name" was too generic. Moving forward, I am okay with using "name". I will be happy to review the PR.

@glemaitre
Copy link
Member Author

Moving forward, I am okay with using "name". I will be happy to review the PR.

Basically, it is to use estimator_name everywhere instead :)

@thomasjpfan
Copy link
Member

Now that we have "from_predictions", there is not always an estimator. So "name" is better now?

@glemaitre
Copy link
Member Author

with from_predictions, you can pass estimator_name as well. If nothing is provided, we default to None.

@thomasjpfan
Copy link
Member

thomasjpfan commented Sep 13, 2021

For example, PrecisionRecallDisplay.from_predictions accepts name and sets the estimator_name=name. I think that inconsistency is weird.

I guess a PR to make CalibrationDisplay to be more like PrecisionRecallDisplay is okay for the time being. It's better to make the change now since CalibrationDisplay has not been officially released yet.

@glemaitre
Copy link
Member Author

PrecisionRecallDisplay.from_predictions does not set estimator_name:

name = name if name is not None else estimator.__class__.__name__

name only allows for overwritting estimator_name.

@glemaitre
Copy link
Member Author

However, CalibrationDisplay.plot is setting self.name and this is weird:

name = self.name if name is None else name
self.name = name

I indeed corrected this in the PR.

@thomasjpfan
Copy link
Member

Ah yes, in that case CalibrationDisplay.plot needs to be fixed.

@thomasjpfan
Copy link
Member

thomasjpfan commented Sep 13, 2021

To be specific, the issue I was thinking about was the user API:

display = PrecisionRecallDisplay.from_predictions(..., name="Model one")

# The current way to get the name of the curve
display.estimator_name

# Is this better?
display.name

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants