8000 Remove validation from __init__ and set_params from SGDOneClassSVM by MrinalTyagi · Pull Request #21824 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

Remove validation from __init__ and set_params from SGDOneClassSVM #21824

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
Closed
wants to merge 6 commits into from
Closed

Remove validation from __init__ and set_params from SGDOneClassSVM #21824

wants to merge 6 commits into from

Conversation

MrinalTyagi
Copy link
Contributor

Reference Issues/PRs

Address: #21406

What does this implement/fix? Explain your changes.

Add type check of alpha in _validation in SGDOneClassSVM class

Any other comments?

Support from someone in case of error in merging will be highly appreciable.

@MrinalTyagi
Copy link
Contributor Author

@glemaitre Could you please guide how to proceed in the following issue? If I let alpha stay in init then it will fail but if I move alpha assignment to _validation_params it gives me a error of setting class attribute alpha is not acceptable.

@ogrisel
Copy link
Member
ogrisel commented Dec 2, 2021

This will likely be complex to fix because of the class hierarchy that implies shared code while enforcing the same name for a reparametrized hyper-parameter...

+1 the backward compat handling that will be tricky to get right. @MrinalTyagi let me suggest to try to tackle another estimator. Getting this one right will open a can of worms.

@MrinalTyagi
Copy link
Contributor Author

@ogrisel Sure. I am willing to contributions to any type of issue or feature addition so you can definitely provide me another estimator as well as other issues if you feel right as I have just started working on open source and wanna continue it.

@arisayosh
Copy link
Contributor

Hi @MrinalTyagi and @ogrisel,

@iofall and I started working on this last week not realizing that a solution had already been drafted. We did the following and it seemed to pass all of the test required:

  1. Changed alpha to None in __init__
  2. Previously alpha was initialized as self.nu / 2 in __init__ which was performed before validation of nu. This caused type errors in the __init__ method.
  3. Now alpha is initialized as None and modified after validation in its respective fit() & partial_fit() methods

Let me know what you think!

@MrinalTyagi
Copy link
Contributor Author

Hi @MrinalTyagi and @ogrisel,

@iofall and I started working on this last week not realizing that a solution had already been drafted. We did the following and it seemed to pass all of the test required:

  1. Changed alpha to None in __init__
  2. Previously alpha was initialized as self.nu / 2 in __init__ which was performed before validation of nu. This caused type errors in the __init__ method.
  3. Now alpha is initialized as None and modified after validation in its respective fit() & partial_fit() methods

Let me know what you think!

Test like the build test or the individual test for this class that runs using pytest sklearn/tests/test_common.py::test_estimators_do_not_raise_errors_in_init_or_set_params

@ogrisel
Copy link
Member
ogrisel commented Dec 7, 2021 •
edited
Loading

@arisayosh I am not sure I understand the details of your plan because when you speak of __init__ you do not specigy which class you are referring to, but it's possible that your strategy can work (but I am afraid that the devil is in the details). Feel free to open a concurrent PR.

@arisayosh
Copy link
Contributor

Test like the build test or the individual test for this class that runs using pytest sklearn/tests/test_common.py::test_estimators_do_not_raise_errors_in_init_or_set_params

Hi @MrinalTyagi, we tested pytest sklearn/tests/test_common.py::test_estimators_do_not_raise_errors_in_init_or_set_params and pytest sklearn/linear_model/tests .

We've opened a concurrent PR here for these changes --> #21944. The alpha is set to None during instantiation of SGDOneClassSVM and set to self.nu/2 during fit() and partial_fit() before self._validate_params().

cc: @ogrisel, @iofall

@jeremiedbb
Copy link
Member

Done in #24433

@jeremiedbb jeremiedbb closed this Mar 2, 2023
@ogrisel ogrisel mentioned this pull request Dec 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
module:linear_model
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MrinalTyagi @ogrisel @arisayosh @jeremiedbb
0