fixed sklearn sanity checks for valid_metrics #925
Conversation
|
Thanks for opening this pull request! We'll make sure it's perfect before merging 🤗 |
neurokit2/complexity/utils.py
Outdated
| @@ -111,7 +111,7 @@ def _get_count( | |||
| # ------------------- | |||
| # Sanity checks | |||
| sklearn_version = version.parse(sklearn.__version__) | |||
| if sklearn_version >= version.parse("1.3.0"): | |||
| if sklearn_version in [version.parse("1.3.0"), version.parse("1.3.0rc1")]: | |||
There was a problem hiding this comment.
shouldn't this be:
if sklearn_version >= version.parse("1.3.0") or sklearn_version == version.parse("1.3.0rc1"):There was a problem hiding this comment.
I tested and we only have the method in versions 1.3.0 and 1.3.0rc1. Versions greater than 1.3.0 reverted back to the attribute style. So the method is only available in those two versions.
There was a problem hiding this comment.
I see, makes sense then thanks!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #925 +/- ##
=======================================
Coverage 54.86% 54.87%
=======================================
Files 303 303
Lines 14255 14255
=======================================
+ Hits 7821 7822 +1
+ Misses 6434 6433 -1
☔ View full report in Codecov by Sentry. |
|
|
Description
This PR aims to fix the sklearn.neighbors.KDTree.valid_metrics() sanity check which has been introduced in skearn.version 1.3.0 (and also in 1.3.0rc1), but has afterward been reverted due to compatibility issues with other libraries. More about this can be found here:
scikit-learn/scikit-learn#26742
scikit-learn/scikit-learn#26754
Proposed Changes
I changed the
if sklearn_version >= version.parse("1.3.0"):condition toif sklearn_version in [version.parse("1.3.0"), version.parse("1.3.0rc1")]:so we can only cover the two cases in whichsklearn.neighbors.KDTree.valid_metricsis not an attribute and is instead a method.