[MRG] Fix warnings during tests #5297
Conversation
| @@ -211,6 +211,12 @@ def _boston_subset(n_samples=200): | |||
| BOSTON = X, y | |||
| return BOSTON | |||
|
|
|||
| def get_kwargs(estimator_class): | |||
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
| @@ -213,8 +213,8 @@ def _boston_subset(n_samples=200): | |||
| return BOSTON | |||
|
|
|||
|
|
|||
| def set_fast_parameters(estimator): | |||
| # speed up some estimators | |||
| def set_optimal_parameters(estimator): | |||
There was a problem hiding this comment.
@amueller I have changed the name of this function to set_optimal_parameters
| @@ -647,8 +648,20 @@ def is_abstract(c): | |||
| return sorted(set(estimators)) | |||
|
|
|||
|
|
|||
| NON_RANDOM_CLASSES = (DBSCAN,) | |||
|
|
|||
|
|
|||
| def set_random_state(estimator, random_state=0): | |||
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. |
| @@ -449,7 +449,7 @@ def test_parallel_classification(): | |||
| decisions2 = ensemble.decision_function(X_test) | |||
| assert_array_almost_equal(decisions1, decisions2) | |||
|
|
|||
| ensemble = BaggingClassifier(SVC(), | |||
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. |
|
I will say something obvious, but I feel it is good to point this out anyway. Fixing for test warnings should be one of the last things to do before release. We are fixing many bugs in these weeks and many imply raising new DeprecationWarnings; some of them in tests might not be caught by the same PR (by mistake). I think this PR can be merged if it achieved what was planned at the beginning. Instead, I will not close #5089 until the very end --maybe only on beta. |
Sure, I think in PR's like this it is all right to have incremental improvements rather than fix all warnings at once. Moreover, merging this would make it easier to quickly filter out the new warnings, while not bothering about the old ones as you have pointed out while bug fixing for the release. Are you still -1 to merging this right now, and dealing with the new warnings as and when we get to them? |
|
@MechCoder , you got my point. Incremental is better. Feel free to merge this please. I would keep #5089 open for a while, as a reminder. |
|
Merged. Thanks. |
|
@giorgiop we wanted to release two 20 days ago, as you can see on the tag. Which is why I asked @vighneshbirodkar to work on this. My health and travel have delayed everything unfortunately. |
Addresses #5089