Skip to content

Less verbose estimator tags mechanism #14892

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Less verbose estimator tags mechanism #14892

wants to merge 2 commits into from

Conversation

rth
Copy link
Member

@rth rth commented Sep 5, 2019 •
edited
Loading

Alternative to #14644

We should pick one of these two PRs.

This uses the current tag estimator mechanism but changes it from,

class Estimator(BaseEstimator):
    def _more_tags(self):
        return {'allow_nan': True}

to a less verbose,

class Estimator(BaseEstimator):
    _more_tags = {'allow_nan': True}

_more_tags can also be a property.

The previous behavior still works with a deprecation warning.

Tried to also address @thomasjpfan's comment from #14884 (comment)

In a follow up, we should document how the order of the inheritance matters now when using tags.

but the description can certainly be improved.

cc @amueller

@rth rth mentioned this pull request Sep 5, 2019
Closed
@NicolasHug
Copy link
Member

So if at least one tag is run-time dependent, _more_tags is an instance attribute. Else, it's a class attribute, is that correct?

I don't know how I feel about that.

Apart from saving 2 lines of code, what's the real benefit?

@amueller
Copy link
Member

amueller commented Sep 5, 2019 •
edited
Loading

Apart from saving 2 lines of code, what's the real benefit?

you mean half a line, if none is runtime dependent and adding half a line if any are?

So if at least one tag is run-time dependent, _more_tags is an instance attribute. Else, it's a class attribute, is that correct?

It's a property if any is runtime dependent and a class attribute otherwise.

@amueller
Copy link
Member

amueller commented Sep 5, 2019

This saves a couple of characters but makes the runtime dependence of the tags implicit. Not sure if that's worth it.
One might argue that it could be interesting in some cases to know whether the tags are determined at runtime or not. That might be an argument for this.

I think it's better to think about what mechanism we want, not how long the code is. I could add a helper in #14644 to make it similarly short, but I'm not sure it would add clarity.
I still think having the base class have explicit control over the derived classes fundamentally inverts the control structure of OO design.

@amueller
Copy link
Member

amueller commented Sep 5, 2019

btw most of the "allow_nan" ones are actually runtime dependent, like they are in KNNImputer. The other imputers don't implement the tag correctly, I think.

@jnothman
Copy link
Member

jnothman commented Sep 6, 2019 via email

@rth
Copy link
Member Author

rth commented Sep 6, 2019

Fair enough. Closing this.

@jnothman are you then +1 for #14644 ? A third alternative is to keep things as they are now.

@rth rth closed this Sep 6, 2019
@rth rth deleted the tags-more-tags branch September 6, 2019 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rth @NicolasHug @amueller @jnothman