10000 TST: Check sklearn ImportError of _index_param_value by GuillaumeFavelier · Pull Request #7173 · mne-tools/mne-python · GitHub
[go: up one dir, main page]
Skip to content

TST: Check sklearn ImportError of _index_param_value #7173

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
Merged
merged 3 commits into from
Jan 5, 2020
Merged

TST: Check sklearn ImportError of _index_param_value #7173

merged 3 commits into from
Jan 5, 2020

Conversation

GuillaumeFavelier
Copy link
Contributor

Follow-up of #7153 (comment), this PR tests how the CIs behave especially Travis and Azure with Python 3.7

@GuillaumeFavelier GuillaumeFavelier mentioned this pull request Jan 3, 2020
Merged
5 tasks
@larsoner
Copy link
Member
larsoner commented Jan 3, 2020

I am not convinced the fix from #7153 is correct. It appears as though _index_param_value has been removed in their master branch:

scikit-learn/scikit-learn#15863

So we're going to need to stop using it, use a try/except pattern of some sort, or port it to fixes.py. My vote would be to do in mne/fixes.py:

def _check_fit_params(...):
    try:
        from sklearn.utils.validation import _check_fit_params as _sklearn_check_fit_params
    except ImportError:
        # copy-paste current sklearn master _check_fit_params code for older sklearn
        ...
    else:
         return _sklearn_check_fit_params(...)

It's future compatible with any improvements they make to _check_fit_params while being backward compatible (hopefully) with older sklearn versions.

But we should see if @agramfort has an opinion.

@codecov
Copy link
codecov bot commented Jan 4, 2020 •
edited
Loading

Codecov Report

Merging #7173 into master will decrease coverage by 0.08%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #7173      +/-   ##
==========================================
- Coverage   89.74%   89.65%   -0.09%     
==========================================
  Files         444      444              
  Lines       79352    79351       -1     
  Branches    12684    12683       -1     
==========================================
- Hits        71212    71143      -69     
- Misses       5353     5402      +49     
- Partials     2787     2806      +19

@agramfort
Copy link
Member
agramfort commented Jan 4, 2020 via email

@agramfort agramfort marked this pull request as ready for review January 5, 2020 15:43
@agramfort
Copy link
Member

ok green. it should make CIs in master green again.

@agramfort agramfort merged commit 1672c60 into mne-tools:master Jan 5, 2020
@GuillaumeFavelier GuillaumeFavelier deleted the hotfix_sklearn_import_error branch January 6, 2020 08:54
AdoNunes pushed a commit to AdoNunes/mne-python that referenced this pull request Apr 6, 2020
@GuillaumeFavelier @agramfort @AdoNunes
TST: Check sklearn ImportError of _index_param_value (mne-tools#7173)
7f2e772 
* TST: Check CIs

* backport sklearn _check_fit_params

* real fix...

Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>
AdoNunes pushed a commit to AdoNunes/mne-python that referenced this pull request Apr 6, 2020
@GuillaumeFavelier @agramfort @AdoNunes
TST: Check sklearn ImportError of _index_param_value (mne-tools#7173)
c3babbd 
* TST: Check CIs

* backport sklearn _check_fit_params

* real fix...

Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>
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
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@GuillaumeFavelier @larsoner @agramfort
0