MAINT: Future compatibility with MNE 1.6 #120
Conversation
Codecov Report
@@ Coverage Diff @@
## main #120 +/- ##
==========================================
- Coverage 90.93% 89.99% -0.95%
==========================================
Files 30 30
Lines 2615 2648 +33
Branches 501 509 +8
==========================================
+ Hits 2378 2383 +5
- Misses 136 155 +19
- Partials 101 110 +9
|
|
Green locally on MNE 1.5 and 1.6, I think I got them all ;) |
|
The most used private import is |
|
Well done! Everything seems to be working, but it's quickly going to be a headache... We'll keep this PR open until the MNE |
|
I think with this PR we can keep the compatibility with MNE above and below 1.6; it's not that much of an headache since the same functions are available in both versions, just in 2 different places. |
|
1c6799d is a miss with numpy 1.25 installed. I'll try again later. |
|
Ok, now we have the correct numpy/scipy/mne versions on that pytest run.
And should be fixed by mne-tools/mne-python#11994 if it's not part of our code. |
There was a problem hiding this comment.
@vferat I added the main changes as review comments. The rest is only some clean-up, sorting, .. nothing important.
Hopefully CIs should be green now, with one bemol I did not solve. Warnings do not fail the CI, and I would have missed that numpy 1.25 deprecation warning if I did not click on the CI details.
If I'm right, warnings do fail my github action workflows with a similar configuration, so I'm not sure why it's not the case with azp here.
Good to merge on my end.
| python -m pip install --progress-bar off --force-reinstall git+https://github.com/mne-tools/mne-python | ||
| python -m pip install --progress-bar off --upgrade --pre --only-binary :all: -i https://pypi.anaconda.org/scientific-python-nightly-wheels/simple --timeout=180 numpy scipy scikit-learn matplotlib |
There was a problem hiding this comment.
That's the one test I changed. Instead of installing only MNE (main), I also install the pre-release version of numpy, scipy, scikit-learn and matplotlib (they need each other to work here, we can't mix a pre-release numpy with the current stable matplotlib or scikit-learn).
| if check_version("mne", "1.6"): | ||
| from mne._fiff.pick import _picks_to_idx | ||
| else: | ||
| from mne.io.pick import _picks_to_idx |
There was a problem hiding this comment.
You already saw that, but that's the syntax I used to load correctly the functions we used. I don't think it's adding a large maintenance burden, and we can keep MNE < 1.6 and >= 1.6.
When we will need to remove it, a simple grep on from mne._fiff or on check_version("mne", "1.6") will find all occurences.
| nchan = int(tag.data) | ||
| nchan = int(tag.data.item()) | ||
| elif kind == FIFF.FIFF_CH_INFO: | ||
| tag = read_tag(fid, pos) | ||
| chs.append(tag.data) | ||
| elif kind == FIFF.FIFF_MNE_CUSTOM_REF: | ||
| tag = read_tag(fid, pos) | ||
| custom_ref_applied = int(tag.data) | ||
| custom_ref_applied = int(tag.data.item()) |
There was a problem hiding this comment.
tag.data is a one element numpy array, and the cast was raising the deprecation warning from numpy.
Fix taken from MNE directly: mne-tools/mne-python#11644
| 'joblib', | ||
| 'matplotlib', | ||
| 'scikit-learn', | ||
| 'mne>=1.2', |
There was a problem hiding this comment.
Bumping MNE minimum requirement to 1.2, since this will be released with MNE 1.6, and keeping compatibility with the 4 last versions of our core dependencies is good practice.
The FIFF I/O was moved to a different module in mne-tools/mne-python#11903