Skip to content

Less verbose estimator tags mechanism#14892

Closed
rth wants to merge 2 commits intoscikit-learn:masterfrom
rth:tags-more-tags
Closed

Less verbose estimator tags mechanism#14892
rth wants to merge 2 commits intoscikit-learn:masterfrom
rth:tags-more-tags

Conversation

@rth
Copy link
Copy Markdown
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
Copy Markdown
Member

NicolasHug commented Sep 5, 2019

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

jnothman commented Sep 6, 2019 via email

@rth
Copy link
Copy Markdown
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