DOC add interval range for parameter of SGDRegressor

[bmgdc]

Reference Issues/PRs

I couldn't find a related issue.

What does this implement/fix? Explain your changes.

The screenshot below is straight from SGDRegressor's documentation.

The sentence "Also used to compute the learning rate when set to learning_rate is set to 'optimal'" in, suffers from multiple issues:

• It's not correct English.
• It's not as informative as it could be with regard to the range of accepted values.
• It doesn't match what is seen in SGDClassifier's documentation.

This pull request fixes all of these.

In addition to the above, multiple constraints to the parameters were included in the documentation of the regressor. A couple of opportunistic improvements were also made (in 8262eba).

Any other comments?

[github-actions]

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

_{Generated for commit: 06c482d. Link to the linter CI: here}

[betatim]

Generally looks good to me. Thanks for putting in the effort to fix this!

One comment about adding the ranges to all parameters. All other changes LGTM.

[glemaitre]

Could you document epsilon with:

        Values must be in the range `[0.0, inf)`.

[glemaitre]

Otherwise LGTM.

[bmgdc]

Could you document epsilon with:

        Values must be in the range `[0.0, inf)`.

Done in b1253fb. Thanks!

[bmgdc]

@betatim @glemaitre

Since the commits addressing the last review, I included two more commits:

• 97d5b4d documents the ranges in SGDOneClassSVM
• d181bda fixes the documented range (for your convenience, here's the constraint) for the average parameter in SGDClassifier and includes it for SGDRegressor and SGDOneClassSVM

I committed in this way to make it easily reversible if you decide that's the way to go.

Thank you for looking into this!

[betatim]

I agree the parameter constraint is [0, n_samples], but I don't understand why. Is using 0 just a different way of saying average=True, as in "start right away"? If yes that is surprising because normally I'd think of 0, in the context of bool as False. So my question is, should we update the constraint to match the doc string (in the text it also says "if set to a value greater than 1")?

Maybe someone with more knowledge can comment.

[glemaitre]

Looking at the code around, I would correct the parameter validation here because we have a lot of code with something like if self.average > 0.

[bmgdc]

@betatim @glemaitre Would you like me to revert the commit that updates the docstrings relative to the average parameter (it's isolated in d181bda) and open an issue regarding average constraints and usage?

[glemaitre]

Indeed this would be easier to handle in a separate PR.

[bmgdc]

@betatim @glemaitre I opened an issue for this here. Feel free to edit it as you see fit.

[betatim]

As far as I can tell the new doc strings you added are consistent with what the parameter checking system uses.

One comment about average that we should resolve.

---

In general it is easier to not "add on" to an existing PR. In this case it is kind of related but also broadens the scope of the PR (to mroe estimators). I think getting a PR, the author and the reviewers all to converge so that the merge button can be clicked turns out to be quite hard, so adding "divergence" by adding on to a PR tends to prevent convergence :D Not a big deal here especially as this is your first time contributing, more soemthing to consider for the future.

[bmgdc]

In general it is easier to not "add on" to an existing PR. In this case it is kind of related but also broadens the scope of the PR (to mroe estimators). I think getting a PR, the author and the reviewers all to converge so that the merge button can be clicked turns out to be quite hard, so adding "divergence" by adding on to a PR tends to prevent convergence :D Not a big deal here especially as this is your first time contributing, more soemthing to consider for the future.

Thank you very much! I'll be mindful of that in the future.

[glemaitre]

LGTM. Let's tackle the average issue in another PR.