8000 DOC Pilot of annotating parameters with category by jnothman · Pull Request #17929 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]

Skip to content

DOC Pilot of annotating parameters with category #17929

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jnothman
Copy link
Member

Reference Issues/PRs

Resolves #17566 more or less.

What does this implement/fix? Explain your changes.

This demonstrates #17566 (comment) just for LogisticRegression.

It badges parameters as being either Control, Tuning or Resources parameters, per @mitar's comment regarding labels used for AutoML in the D3M project.

So far these labels add coloured badges, and link to glossary entries.

They are added in the docstring after a parameter's entire description, before .. versionadded etc., in a new paragraph. Is this the right place?

image

Any other comments?

I think this is a minimal patch to demonstrate the idea, and then to invite contributors to label other parameters throughout the library. Extensions include:

  • The labels should probably be positioned elsewhere (e.g. inset to the right of the parameter name), and this could be achieved with Javscript/DOM games.
  • It would be nice to allow the user to only display parameters with a particular badge. This filtering again could be achieved with JavaScript.
  • @mitar hoped for these annotations to be available as machine-readable metadata. We could achieve this with numpydoc.

@thomasjpfan
Copy link
Member

hoped for these annotations to be available as machine-readable metadata. We could achieve this with numpydoc.

We can have machine readable metadata with typing: #17799

For example:

dual: Annotated[bool, Tuning] = False

@jnothman
Copy link
Member Author

We can have machine readable metadata with typing: #17799

Hmm. Indeed. I can't say that I like how Annotated[bool, Tuning] looks. I think it will distract from the type spec for many users.
But I also wouldn't want this info stored in two places if it could be helped...

I'm interested to hear your thoughts on which is the right way forward, and if Annotated, how to make it more human-readable.

@thomasjpfan
Copy link
Member

But I also wouldn't want this info stored in two places if it could be helped...

In #17799, I am moving toward having the type in two places: in the docstring and in the annotation. Then have a linter make sure they are consistent. Regarding this PR, we can go down this path as well.

I'm interested to hear your thoughts on which is the right way forward, and if Annotated, how to make it more human-readable.

With the above approach, the docstring will be the "human-readable" version of the metadata and the type annotation will be the "machine-readable" version.

(I started with a version with a single source of truth in the typing annotation. It injected the typing information into __doc__ during module loading time, but it increased the time it took the load the module 10x.)

@@ -1043,6 +1043,8 @@ class LogisticRegression(LinearClassifierMixin,
only supported by the 'saga' solver. If 'none' (not supported by the
liblinear solver), no regularization is applied.

|ControlParameter|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I would say this is a (even traditional) example of a tuning parameter?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... I suppose you're right.

I suspect that I over generated control parameters.

fit_intercept : bool, default=True
Specifies if a constant (a.k.a. bias or intercept) should be
added to the decision function.

|ControlParameter|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This as well. It does not influence the shape of the output, for example.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And what of class_weight?

Copy link
Contributor
@mitar mitar Jul 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say that one is hard to tune, like, I have not heard about one tuning that with exact values for classes. But for "balanced" vs. None, I think I would say you could try your pipeline with both and see what is the best, no? So tune?

So in a way, in my mental model, I see tuning like "can I fiddle with this value without having to change the rest of the pipeline". :-) (There are some dependencies between parameters though, which I might have to fix as a consequence.)

.. versionadded:: 0.17
*class_weight='balanced'*

random_state : int, RandomState instance, default=None
Used when ``solver`` == 'sag', 'saga' or 'liblinear' to shuffle the
data. See :term:`Glossary <random_state>` for details.

|ControlParameter|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one of those which might end up better having a completely custom type of a parameter, like RandomSourceParameter or something.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RandomSourceParameter also illustrates the limits of such approach: I have no idea what this means.

If we try to be too precise, we will make it tedious to contribute, and will not reach any users as they won't understand.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe random_state should just not have any label?

warm_start : bool, default=False
When set to True, reuse the solution of the previous call to fit as
initialization, otherwise, just erase the previous solution.
Useless for liblinear solver. See :term:`the Glossary <warm_start>`.

|ResourcesParameter|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be double tagged as control as well. So this drastically changes behavior of the estimator if you are calling it multiple times.

@@ -1140,11 +1162,15 @@ class LogisticRegression(LinearClassifierMixin,
For the liblinear and lbfgs solvers set verbose to any positive
number for verbosity.

|ResourcesParameter|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think sklearn might need also DebugParameter type as well. (We do not have those in D3M.)

@mitar
Copy link
Contributor
mitar commented Jul 18, 2020

Nice work. For reference, here you can find how we annotated each hyper-parameter in D3M program. Note that it is not perfect either, which shows some if the issue with determining where each hyper-parameter belongs.

@mitar
Copy link
Contributor
mitar commented Jul 18, 2020

In practice, I think many parameters are tuning. To me, main example of a control one is add_indicator parameter, which completely changes the shape of the output. And probably most parameters of to the ColumnTransformer.

@justmarkham
Copy link
Contributor

I love this concept! I was teaching GridSearchCV recently, and one of my students asked: "Where do I go to figure out which parameters to tune?" My response was: "The scikit-learn documentation tells you what the parameters are, but it doesn't tell you which ones you should tune." Thus, it's exciting to find that parameter annotation is being considered!

However, I would like to propose a slightly different solution. Rather than attempting to annotate every parameter, perhaps only annotate the "most important" tuning parameters using the label "Important Tuning Parameter". In the LogisticRegression example, perhaps only penalty and C would be tagged as an important tuning parameter, and the rest of the parameters would remain without tags.

From my perspective, here are the benefits of only tagging the "most important" tuning parameters:

  1. It's a lot of work to figure out how every parameter for every model should be tagged. This is likely to generate a lot of discussion, followed by many PRs (once the annotations are live) from people who believe that certain parameters have been tagged incorrectly.
  2. It's easier to come to an agreement about what the "most important" tuning parameters are. You can ask experts, you can review papers, or you can just look around at what "most people" are tuning. And it's okay if a parameter that can be tuned is not marked as an "important tuning parameter", because scikit-learn isn't saying that you can't tune it or you shouldn't tune it... scikit-learn is just saying that it's not the "most important" parameter to tune.
  3. It's the most useful option for a novice user. If I'm a novice user and I show up at the scikit-learn page for LogisticRegression, I'm overwhelmed if you tell me that there are 7 tuning parameters. I may not know what values to try, I may not know which parameter combinations are illegal, and I probably don't have the computing power to try all of the combinations. But if I see just 2 parameters marked as "important tuning parameters", then I have an easy starting point!

Anyway, that is my perspective, though I'd welcome feedback from @mitar @jnothman or others!

@GaelVaroquaux
Copy link
Member

Rather than attempting to annotate every parameter, perhaps only annotate the "most important" tuning parameters using the label "Important Tuning Parameter"

I think that this is an interesting proposal.

@GaelVaroquaux
Copy link
Member

I looked at the code and overall I like the idea. A couple of questions:

  • How do we expose this to downstream libraries (for instance for AutoML)?
  • How do we make sure that these annotations are contributed on the new estimators (maybe it's just a matter of documenting it in the contribution guide, but I worry that it may not be enough).

Thanks!!

@jnothman
Copy link
Member Author
jnothman commented Jul 26, 2020 via email

@amueller
Copy link
Member

From the dev call today: we might use a single label "Tune me" (or something like that) that highlights the important things to tune.

Another distinction that @jnothman and I were interested in was distinguishing those that specify the model vs those that optimize it. I'm not entirely sure if in LogisticRegression penalty is important to tune or not?

@NicolasHug
Copy link
Member

We also discussed the possibility of making these annotations machine-readable. If we do, I'd suggest not supporting any kind of backward compatibility, since we might want to change the labels in the future.

@GaelVaroquaux
Copy link
Member
GaelVaroquaux commented Jul 27, 2020 via email

@justmarkham
Copy link
Contributor

From the dev call today: we might use a single label "Tune me" (or something like that) that highlights the important things to tune.

My current personal preference is either "Tuning Parameter" or "Important Tuning Parameter". Those feel like labels to me, whereas "Tune me" feels like a directive.

Another distinction that @jnothman and I were interested in was distinguishing those that specify the model vs those that optimize it. I'm not entirely sure if in LogisticRegression penalty is important to tune or not?

I suggest tuning both penalty and C.

I think I understand the distinction you are drawing between "those that specify the model vs those that optimize it" (with penalty being a specification parameter and C being an optimization parameter, right?), but I'm not sure the end user would benefit from those two categories having different labels.

At the end of the day, my thinking is that the average end user just wants the documentation to answer the question, "Which parameters should I tune in a grid search?"

@jnothman
Copy link
Member Author
jnothman commented Jul 27, 2020 via email

@mitar
Copy link
Contributor
mitar commented Aug 18, 2020

Rather than attempting to annotate every parameter, perhaps only annotate the "most important" tuning parameters using the label "Important Tuning Parameter"

Yes, in our system we have then additional metadata which allow authors to specify a shortlist of hyper-parameters to tune. So my proposal would be to have both tags here: a) all hyper-parameters which are tunable b) hyper-parameters which are reasonable to tune. So having multiple tags allows you to simply provide both tags when needed, or just one.

Base automatically changed from master to main January 22, 2021 10:52
@UnixJunkie
Copy link
Contributor

I vote for a CriticalParameter tag, to flag parameters which strongly impact the performance of a model.

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

Successfully merging this pull request may close these issues.

API split "model" parameters from "config" params.
8 participants
0