[MRG] Fix warnings during tests#5297
[MRG] Fix warnings during tests#5297vighneshbirodkar wants to merge 11 commits intoscikit-learn:masterfrom
Conversation
sklearn/utils/estimator_checks.py
Outdated
There was a problem hiding this comment.
ping @amueller @ogrisel @MechCoder
Some tests warned because the SVM object wasn't initialized with the correct arguments. Is there a better way to fix this ?
There was a problem hiding this comment.
I think if anything, this should be in set_fast_parameters and that function be renamed.
For the common tests it might be ok to catch the deprecation warnings when the decision function is used.
There was a problem hiding this comment.
Either solution is fine, I just don't want to add another helper function here.
2766c97 to
8f1ec1c
Compare
sklearn/utils/estimator_checks.py
Outdated
There was a problem hiding this comment.
@amueller I have changed the name of this function to set_optimal_parameters
sklearn/utils/testing.py
Outdated
There was a problem hiding this comment.
I am wondering if it is OK to do it here. The other alternative is to set random_state to None is get_optimal_parameters and ensure it is always called after set_random_state
There was a problem hiding this comment.
If there is just one class that it is deprecated for, it might be better to just do
if isinstance(estimator, DBSCAN):
return
There was a problem hiding this comment.
(And maybe move the classes for whom sentence to a comment)
|
This is redundant with some fixes in #5277. Have you checked it? |
|
@TomDLT I did now. I can rebase if that is merged before this PR. |
There was a problem hiding this comment.
Just a note for future: There seems to be a runtime warning due to division by zero.
There was a problem hiding this comment.
You mean due to the changes I have made ?
There was a problem hiding this comment.
Nope, not due to your changes. Some independent warning.
|
LGTM (Apart from the inline comments). I am thinking if it would be good to have a custom |
+1 but for another PR :) |
|
I copied my comment to another issue. |
|
LGTM Would it be too wise to add a comment whenever using |
|
Will merge after tests pass. |
Addresses #5089