Skip to content
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.

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

MAINT: Harmonize deps #433

Merged
merged 21 commits into from
Jan 28, 2022
Merged

MAINT: Harmonize deps #433

merged 21 commits into from
Jan 28, 2022

Conversation

larsoner
Copy link
Member

@larsoner larsoner commented Jan 26, 2022

I made requirements.txt and environment.yml look more like MNE-Python's. Let's see what fails and how to fix it.

Works toward #420 by at least getting CIs and examples to work again. Ideally the translation from invalid to valid codes would perhaps be automatic.

Closes #431
Closes #403

@codecov
Copy link

codecov bot commented Jan 26, 2022

Codecov Report

Merging #433 (ebf5c2c) into main (86d0050) will increase coverage by 0.12%.
The diff coverage is 99.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #433      +/-   ##
==========================================
+ Coverage   95.58%   95.71%   +0.12%     
==========================================
  Files          62       63       +1     
  Lines        2356     2404      +48     
  Branches      304      311       +7     
==========================================
+ Hits         2252     2301      +49     
+ Misses         48       47       -1     
  Partials       56       56              
Impacted Files Coverage Δ
...erimental_design/tests/test_experimental_design.py 100.00% <ø> (ø)
mne_nirs/simulation/tests/test_simulation.py 100.00% <ø> (ø)
mne_nirs/io/snirf/tests/test_snirf.py 96.25% <66.66%> (+1.25%) ⬆️
mne_nirs/conftest.py 100.00% <100.00%> (ø)
.../audio_or_visual_speech/_audio_or_visual_speech.py 100.00% <100.00%> (ø)
...datasets/block_speech_noise/_block_speech_noise.py 100.00% <100.00%> (ø)
...rs/datasets/fnirs_motor_group/fnirs_motor_group.py 100.00% <100.00%> (ø)
mne_nirs/io/fold/_fold.py 88.75% <100.00%> (ø)
mne_nirs/io/fold/tests/test_fold.py 100.00% <100.00%> (ø)
mne_nirs/preprocessing/_mayer.py 97.87% <100.00%> (ø)
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86d0050...ebf5c2c. Read the comment docs.

@larsoner
Copy link
Member Author

Looks like the pandas problems were really exposing issues where keys were not available, mostly because of nilearn's new requirement that variables have proper names, e.g., don't have / or . in their names. I've refactored the code to raise errors earlier in a few places where you request a condition that does not exist, and refactored examples and tests quite a bit to avoid using illegal names. While I was at it, I simplified a bit some of the filterwarnings filtering stuff, added a conftest.py, and added some requires_<module> decorators as I hit requirements I didn't have.

One particularly problematic dependency seems to be lets_plot -- they don't have a 3.10 wheel, and locally trying to build and install their code is a bit of a nightmare. @rob-luke is it feasible to make use of seaborn or something other than lets_plot to lighten the dependency load for the two examples that use it?

@larsoner
Copy link
Member Author

I see my lets-plot gripe is already in #397 so no need to discuss here :)

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jan 26, 2022

This pull request introduces 1 alert when merging 8fa4b99 into 86d0050 - view on LGTM.com

new alerts:

  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jan 27, 2022

This pull request introduces 1 alert when merging 9956bd4 into 86d0050 - view on LGTM.com

new alerts:

  • 1 for Module is imported with 'import' and 'import from'

@rob-luke
Copy link
Member

Thanks for taking a look at this @larsoner

Looks like the pandas problems were really exposing issues where keys were not available, mostly because of nilearn's new requirement that variables have proper names, e.g., don't have / or . in their names.

I thought those changes hadn't been released yet and we had some time to fix that, sorry I should have realised this then. The other fixes are also greatly appreciated.

I've refactored the code to raise errors earlier in a few places where you request a condition that does not exist, and refactored examples and tests quite a bit to avoid using illegal names.

Thanks. And I have started #403 to try and fix this issues, but there may be a better way and I haven't worked on it for a bit.

@rob-luke
Copy link
Member

It looks like nilearn is starting to prep for their next release 0.9, so this might become an issue sooner rather than later. nilearn/nilearn#3143

@larsoner
Copy link
Member Author

I thought those changes hadn't been released yet and we had some time to fix that

And

It looks like nilearn is starting to prep for their next release 0.9, so this might become an issue sooner rather than later. nilearn/nilearn#3143

Indeed :) The idea in MNE-Python (and here, with all the --pre and main / master-branch deps used in CIs) is that sooner or later we'll have to deal with upstream package changes. If we don't make some CIs run the latest-and-"greatest" versions of all dependencies before they're actually released via pip and conda then we only find out when CIs mysteriously start failing with pip/conda install <package>. However, at that point it's already too late (ideally) to deal with the issue because users can feel this pain and get failures rather than devs who can iterate over days (or weeks?) to make things work across the newest and the oldest (supported) versions simultaneously. (Incidentally, this is also why MNE-Python has an old/compat GH actions run as well, to test our oldest acceptable dependency versions.)

And I have started #403

Sorry I actually missed this -- it looks like the /-to-_ solution here isn't too different from a /-to-__ solution there. I think ideally MNE-NIRS would maybe automagically do some of this, but the goal of this PR is really to punt on this by doing some inline sanitizing for the time being like:

raw.annotations.description[:] = [d.replace('/', '_') for d in raw.annotations.description]

This PR I think is very close to working. I plan to keep iterating about 8h from now so @rob-luke if you have some solution in the meantime for this one on CircleCI:

Unexpected failing examples:
/home/circleci/project/examples/general/plot_16_waveform_group.py failed leaving traceback:
Traceback (most recent call last):
  File "/home/circleci/project/examples/general/plot_16_waveform_group.py", line 380, in <module>
    roi_model = smf.mixedlm("Value ~ Condition", input_data,
  File "/home/circleci/python_env/lib/python3.8/site-packages/statsmodels/regression/mixed_linear_model.py", line 1046, in from_formula
    mod = super(MixedLM, cls).from_formula(
  File "/home/circleci/python_env/lib/python3.8/site-packages/statsmodels/base/model.py", line 206, in from_formula
    raise ValueError('endog has evaluated to an array with multiple '
ValueError: endog has evaluated to an array with multiple columns that has shape (10, 10). This occurs when the variable converted to endog is non-numeric (e.g., bool or str).

I'm all ears -- I'm assuming it's some pandas<->statsmodels issue since looking at the dataframe and model definition, it seems like it should work (but at the same time I can replicate locally with a conda env similar to the one in the mne sys_info of the CI...

Also this PR turns warnings into errors, which is useful for catching DeprecationWarning's like "append is deprecated, use concat instead".

@larsoner
Copy link
Member Author

Updated hash to account for rob-luke/BIDS-NIRS-Tapping#4 being merged

@larsoner
Copy link
Member Author

All green, feel free to review and merge if you're happy @rob-luke !

@rob-luke
Copy link
Member

at that point it's already too late (ideally) to deal with the issue because users can feel this pain and get failures rather than devs

Makes total sense, thanks for explaining.

PR looks great! That's quite some work, thanks. I think you missed one change in the FIR tutorial and I wrote a brief explanation of the sanitisation in the first occurrence. I couldnt make the edits in the web interface, so I have made a PR to your PR in larsoner#1

@larsoner
Copy link
Member Author

@rob-luke it looks like your PR into my branch broke some stuff. Want to push directly to this branch to fix it?

@rob-luke
Copy link
Member

rob-luke commented Jan 28, 2022

We also hit another dependency API change with pysnirf2, not our lucky week. But all green now, thanks @larsoner!!

@rob-luke rob-luke merged commit f9aa7bd into mne-tools:main Jan 28, 2022
@rob-luke
Copy link
Member

rob-luke commented Jan 28, 2022

I dont think this closes #403. This PR just avoids the problem, I think it would be nice to automatically sanitise annotation names with a / somehow. It doesnt necessarily have to be as done in #403, but using HED style annotations would be handy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants