Repository navigation
[MRG] MAINT: compatibility sklearn 0.20 - #526
Conversation
|
To me it seems that the test-cases should work again (with sklearn 0.18) Please see this travis run. I restarted the tests, but they still crash. Anything sklearn > 0.18? |
|
One more crashing apparently: |
| transformer_list=[('ohe', ohe), ('scaler', scaler)]) | ||
| serialization = sklearn_to_flow(fu) | ||
| estimators_names = ('FeatureUnion', 'OneHotEncoder', 'StandardScaler') | ||
| for keyword in estimators_names: |
There was a problem hiding this comment.
What's the reason to make this test more leniant?
There was a problem hiding this comment.
We moved some of the transformer from module. Therefore, the serialization name will be incorrect across version of scikit-learn. In the future, it could be as well the case.
I have 2 rermarks:
- The test was intended to check the presence of the estimator in the serialization, doesn't it? Is it something else in the test which was important to be tested?
- Speaking with @janvanrijn IRL, currently all scikit-learn estimators are imported with only a single level and no more. All extra level are actually kinda private (some are explicit by putting some "_" at the front of the module name). I think that you should not rely on those extra levels.
To be more specific, you should rely that a user do from sklearn.decompisition import PCA and not from sklearn.decomposition.pca import PCA since pca should be private module.
There was a problem hiding this comment.
- The test also intends to check that the name is correctly serialized and does not change (also due to mistakes on our side).
- Regarding the names: they extra module level might be private, but that's the information we get from the Python interpreter when we ask it for the full module path. Do you have any other suggestion on how to obtain the name(s)?
There was a problem hiding this comment.
OK I see and it makes sense. I don't have a straightforward solution.
The function sklearn.utils.testing could be slightly change or derived to get a mapping from the class name of each estiamtor and the shortest import path. Right now we are using getmembers: https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/utils/testing.py#L671
We could think of a try except to get the higher level of import which should not change across version and create this mapping. WDYT?
NB: regarding my previous statement of 2 levels import, I was wrong. We have some estimators which are leaving in the 3rd level (e.g. for feature_extraction for instance).
There was a problem hiding this comment.
I think the idea of having such a mapping is a good idea from the point of view of the OpenML connector, but it needs to live inside of scikit-learn in order to get constant maintenance. For now I suggest to open a new issue in the OpenML-python issue tracker and the scikit-learn issue tracker to keep track of this and get this PR merged. Would you be okay to re-add the string asserts and add to add an if/else statement?
| 'ohe=sklearn.preprocessing.data.OneHotEncoder)') | ||
| estimators_names = ('FeatureUnion', 'OneHotEncoder') | ||
| for keyword in estimators_names: | ||
| self.assertTrue(keyword in estimators_names) |
There was a problem hiding this comment.
I think the same question as above applies here.
| estimators_names = ('FeatureUnion', 'OneHotEncoder', 'StandardScaler') | ||
| for keyword in estimators_names: | ||
| self.assertTrue(keyword in fu1_serialization.name) | ||
| self.assertTrue(keyword in fu2_serialization.name) |
| 'StandardScaler', 'AdaBoostClassifier', | ||
| 'DecisionTreeClassifier') | ||
| for keyword in estimator_names: | ||
| self.assertTrue(keyword in serialized.name) |
|
@mfeurer I answer to the questions. Regarding the less restrictive test, I would be inclined to modify it but I would need a bit more details regarding what actually we should tests there. |
|
Ok I'll do that.
Sent from my phone - sorry to be brief and potential misspell.
|
3 participants
Make test pass with scikit-learn 0.20