-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
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
base: main
Are you sure you want to change the base?
Conversation
Resolves scikit-learn#17566 more or less
We can have machine readable metadata with typing: #17799 For example: dual: Annotated[bool, Tuning] = False |
Hmm. Indeed. I can't say that I like how I'm interested to hear your thoughts on which is the right way forward, and if Annotated, how to make it more human-readable. |
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.
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 |
@@ -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| |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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| |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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| |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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| |
There was a problem hiding this comment.
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| |
There was a problem hiding this comment.
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.)
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. |
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. |
I love this concept! I was teaching 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 From my perspective, here are the benefits of only tagging the "most important" tuning parameters:
Anyway, that is my perspective, though I'd welcome feedback from @mitar @jnothman or others! |
I think that this is an interesting proposal. |
I looked at the code and overall I like the idea. A couple of questions:
Thanks!! |
If we accept a numpydoc dependency (it is already a testing and doc
dependency), then it should be easy to expose automatically as some kind of
attribute, or more explicitly through a utility function designed to
extract these annotations.
I would like to hope that these will be highly visible and will gain their
own traction: users will expect them to be there and be consistent, and
contributions will come to add them to new estimators if otherwise
forgotten. If we want linter-style assurances that each estimator is
covered, I don't think that would be difficult.
|
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 |
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. |
I'm not entirely sure if in LogisticRegression penalty is important to tune or not?
Choose l2 and tune it over 3/4 values (.01, .1, 1, 10), IMHO.
|
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.
I suggest tuning both I think I understand the distinction you are drawing between "those that specify the model vs those that optimize it" (with 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?" |
Yes, I described the distinction between specification and optimisation as
"does it produce a substantially different hypothesis space"... Arguably,
`penalty` does. It also produces a model with different properties, wrt
sparsity and how you might then treat the coefficients, than does C.
I feel like we've fallen short of a clear resolution here.
|
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. |
I vote for a CriticalParameter tag, to flag parameters which strongly impact the performance of a model. |
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?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: