[go: up one dir, main page]
Skip to content
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

Apparent mismatch between possible arguments for average in the base stochastic gradient class #28389

Closed
bmgdc opened this issue Feb 8, 2024 · 5 comments · Fixed by #28582
Closed

Apparent mismatch between possible arguments for average in the base stochastic gradient class #28389

bmgdc opened this issue Feb 8, 2024 · 5 comments · Fixed by #28582
Labels
Bug Validation related to input validation

Comments

@bmgdc
Copy link
Contributor
bmgdc commented Feb 8, 2024 •
edited
Loading

Describe the bug

Raised in #28373 (comment).

The average parameter in BaseSGD (which propagates to SGDRegressor, SGDClassifier and SGDOneClassSVM) is constrained to non-negative integers or boolean values: "average": [Interval(Integral, 0, None, closed="left"), bool, np.bool_].

This seems to be at odds with average=0 seemingly meaning average=True which contradicts the typical truth-evaluation of 0.

Steps/Code to Reproduce

Expected Results

Actual Results

Versions

This is in the main branch right now.
@bmgdc bmgdc added Bug Needs Triage Issue requires triage labels Feb 8, 2024
@bmgdc bmgdc mentioned this issue Feb 8, 2024
Merged
@jeremiedbb
Copy link
Member

Thanks for the report @bmgdc. Looking at the code, average=0 seems to mean average=False since averaging is only triggered by if self.average > 0. So to me the constraint is not confusing. I wouldn't mind that the interval starts at 1 but I guess we let 0 for convenience since the bool constraint doesn't treat 0 as a boolean (isinstance(0, bool) -> False).

@jeremiedbb jeremiedbb removed the Needs Triage Issue requires triage label Mar 4, 2024
@jeremiedbb
Copy link
Member

ping @glemaitre @betatim who originaly discussed that in #28373 (comment), what's your opinion ?

@jeremiedbb jeremiedbb added the Validation related to input validation label Mar 4, 2024
@glemaitre
Copy link
Member

Maybe this is better to keep as-is. Otherwise, we might start to break some code and request a boolean in the previous case of 0. This is maybe not worth it.

We might want to have a comment just to document this legacy behaviour.

@jeremiedbb
Copy link
Member

or deprecate 0 ? Since we deprecated integers for the boolean constraint to make boolean parameters require an true boolean, deprecating 0 would make it consistent with the rest of the code base.

@glemaitre
Copy link
Member

So let's make it consistent then and ask to pass False instead of 0

@jeremiedbb jeremiedbb mentioned this issue Mar 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Validation related to input validation
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

API Deprecate average=0 in SGD jeremiedbb/scikit-learn
3 participants
@glemaitre @bmgdc @jeremiedbb