Skip to content

Conversation

@ChidiRnweke
Copy link

Reference Issues/PRs

In reference to issue #24651. The original issue is updated with the relevant benchmarks in the comments.

What does this implement/fix? Explain your changes.

Sets the default value to "auto" and selects if the problem needs to be solved in primal or dual.

Any other comments?

@glemaitre glemaitre changed the title Set primal or dual for LinearSVR/SVC automatically API add option dual="auto" for LinearSVC/LinearSVR Oct 23, 2022
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

In addition, we will need some tests. Since we will run some deprecation, we need to be sure that we raise the proper warning regarding the change of the default in the future.

In addition, we need to check that "auto" does what we expect.

and ``loss='hinge'`` is not supported.
dual : bool, default=True
dual : {"auto", False, True}, default= "auto"
Copy link
Member

Choose a reason for hiding this comment

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

I think that we still need to keep the default as-is and make a deprecation cycle to change the default in 1.4

Suggested change
dual : {"auto", False, True}, default= "auto"
dual : "auto" or bool, default=True

Comment on lines +43 to +44
Sets dual = False when n_samples > n_features if dual is "auto"
and dual = True otherwise.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Sets dual = False when n_samples > n_features if dual is "auto"
and dual = True otherwise.
`dual="auto"` is equivalent to `dual=False` when
`n_samples > n_features` and `dual=True` otherwise.

optimization problem.
Sets dual = False when n_samples > n_features if dual is "auto"
and dual = True otherwise.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.. versionchanged:: 1.4
The default value will change from `True` to `"auto"` in 1.4.

loss="squared_hinge",
*,
dual=True,
dual="auto",
Copy link
Member

Choose a reason for hiding this comment

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

Comment on lines +373 to +374
Sets dual = False when n_samples > n_features if dual is "auto"
and dual = True otherwise.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Sets dual = False when n_samples > n_features if dual is "auto"
and dual = True otherwise.
`dual="auto"` is equivalent to `dual=False` when
`n_samples > n_features` and `dual=True` otherwise.
.. versionchanged:: 1.4
The default value will change from `True` to `"auto"` in 1.4.

(and therefore on the intercept) intercept_scaling has to be increased.
dual : bool, default=True
dual : {"auto", False, True}, default= "auto"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
dual : {"auto", False, True}, default= "auto"
dual : "auto" or bool, default=True

@glemaitre
Copy link
Member

The CI tell us that we need to modify the _parameter_constraint to show that we accept "auto". We also need to add a hidden option "warn" during the deprecation cycle.

We also need to acknowledge the change in the changelog by adding an entry in doc/whats_new/v1.2.rst.

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.

3 participants