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

BUG: updates for MPL 3.7 compatibility #11409

Merged
merged 11 commits into from
Feb 23, 2023
Merged

Conversation

drammock
Copy link
Member

@drammock drammock commented Jan 6, 2023

closes #11332

So far this is just proof of concept that the new MPL RadioButtons API can do what we need it to do. Tested against this commit in this open MPL PR. The radio buttons look OK:

Screenshot_2023-01-06_15-18-01

...but our current min version of MPL is 3.1 and MPL 3.7 isn't even out yet (probably end of January according to @tacaswell). So we should talk about how we want to handle this; presumably some combination/sequence of

but I'm not sure on the timing of each step.

@drammock drammock added the needs-discussion issues requiring a dev meeting discussion before the way forward is clear label Jan 6, 2023
@tacaswell
Copy link

Pinning back won't help you because pip will just pick an older version of mne-python without the pinning.

@drammock
Copy link
Member Author

drammock commented Jan 6, 2023

Pinning back won't help you because pip will just pick an older version of mne-python without the pinning.

I think that's possible if a user already has MPL 3.7 and then pip installs mne (or requests both MPL and MNE in the same pip install line). But our default install instructions steer users to conda/mamba or a standalone executable installer that uses conda/mamba under the hood, so I think pinning in the environment.yml file it ought to work for those install routes anyway. Also in a fresh environment even the pip install route ought to pick up a pin from requirements.txt as long as they pip install mne and don't already have MPL in the env.

@drammock
Copy link
Member Author

notes to self:

tutorials/preprocessing/50

Traceback (most recent call last):
  File "/home/circleci/project/tutorials/preprocessing/50_artifact_correction_ssp.py", line 183, in <module>
    fig = epochs.average().plot_topomap(times, proj='interactive')
  File "/home/circleci/project/mne/evoked.py", line 435, in plot_topomap
    return plot_evoked_topomap(
  File "/home/circleci/project/mne/viz/topomap.py", line 1826, in plot_evoked_topomap
    _draw_proj_checkbox(None, params)
  File "/home/circleci/project/mne/viz/utils.py", line 487, in _draw_proj_checkbox
    for rect in proj_checks.rectangles:
  File "/home/circleci/python_env/lib/python3.10/site-packages/matplotlib/_api/deprecation.py", line 158, in __get__
    emit_warning()
  File "/home/circleci/python_env/lib/python3.10/site-packages/matplotlib/_api/deprecation.py", line 193, in emit_warning
    warn_deprecated(
  File "/home/circleci/python_env/lib/python3.10/site-packages/matplotlib/_api/deprecation.py", line 96, in warn_deprecated
    warn_external(warning, category=MatplotlibDeprecationWarning)
  File "/home/circleci/python_env/lib/python3.10/site-packages/matplotlib/_api/__init__.py", line 388, in warn_external
    warnings.warn(message, category, stacklevel)
matplotlib._api.deprecation.MatplotlibDeprecationWarning: The rectangles attribute was deprecated in Matplotlib 3.7 and will be removed two minor releases later. Any custom property styling may be lost.

tutorials/inverse/20

Traceback (most recent call last):
  File "/home/circleci/project/tutorials/inverse/20_dipole_fit.py", line 25, in <module>
    from nilearn.plotting import plot_anat
  File "/home/circleci/python_env/lib/python3.10/site-packages/nilearn/plotting/__init__.py", line 47, in <module>
    from . import cm
  File "/home/circleci/python_env/lib/python3.10/site-packages/nilearn/plotting/cm.py", line 226, in <module>
    _cm.register_cmap(name=k, cmap=v)
  File "/home/circleci/python_env/lib/python3.10/site-packages/matplotlib/_api/deprecation.py", line 199, in wrapper
    emit_warning()
  File "/home/circleci/python_env/lib/python3.10/site-packages/matplotlib/_api/deprecation.py", line 193, in emit_warning
    warn_deprecated(
  File "/home/circleci/python_env/lib/python3.10/site-packages/matplotlib/_api/deprecation.py", line 96, in warn_deprecated
    warn_external(warning, category=MatplotlibDeprecationWarning)
  File "/home/circleci/python_env/lib/python3.10/site-packages/matplotlib/_api/__init__.py", line 388, in warn_external
    warnings.warn(message, category, stacklevel)
matplotlib._api.deprecation.MatplotlibDeprecationWarning: The register_cmap function was deprecated in Matplotlib 3.7 and will be removed two minor releases later. Use ``matplotlib.colormaps.register(name)`` instead.

tutorials/clinical/30

Traceback (most recent call last):
  File "/home/circleci/project/tutorials/clinical/30_ecog.py", line 150, in <module>
    rgba = cm.get_cmap("viridis")
  File "/home/circleci/python_env/lib/python3.10/site-packages/matplotlib/_api/deprecation.py", line 199, in wrapper
    emit_warning()
  File "/home/circleci/python_env/lib/python3.10/site-packages/matplotlib/_api/deprecation.py", line 193, in emit_warning
    warn_deprecated(
  File "/home/circleci/python_env/lib/python3.10/site-packages/matplotlib/_api/deprecation.py", line 96, in warn_deprecated
    warn_external(warning, category=MatplotlibDeprecationWarning)
  File "/home/circleci/python_env/lib/python3.10/site-packages/matplotlib/_api/__init__.py", line 388, in warn_external
    warnings.warn(message, category, stacklevel)
matplotlib._api.deprecation.MatplotlibDeprecationWarning: The get_cmap function was deprecated in Matplotlib 3.7 and will be removed two minor releases later. Use ``matplotlib.colormaps[name]`` or ``matplotlib.colormaps.get_cmap(obj)`` instead.

@larsoner
Copy link
Member

@drammock usually I think we fix in main + backport to latest release (patch version numbers are easy enough to push out). If we can do it without it taking too much time I'd rather do that instead of all the pinning work.

@cbrnr
Copy link
Contributor

cbrnr commented Feb 22, 2023

Shouldn't we drop a hotfix release (v1.3.1) and pin Matplotlib < 3.7? The annotation window is currently broken.

@larsoner
Copy link
Member

To me it's a matter of timing, if we can fix this in a day or two we should do it, backport, and release. If we can't, then we should go through the pinning stuff

@cbrnr
Copy link
Contributor

cbrnr commented Feb 22, 2023

Agreed. @drammock do you think this is doable within a day or two? Let me know if I can help.

@drammock
Copy link
Member Author

drammock commented Feb 22, 2023 via email

@larsoner
Copy link
Member

That timeline is okay with me

@larsoner larsoner marked this pull request as ready for review February 22, 2023 21:00
* upstream/main: (46 commits)
  Fix docstrings by replacing str with path-like and fix double backticks for formatting (mne-tools#11499)
  Use pathlib.Path instead of os.path to handle files and folders [circle deploy] (mne-tools#11473)
  MAINT: Fix Circle [circle deploy] (mne-tools#11497)
  MAINT: Use mamba in CIs (mne-tools#11471)
  Updating documentation to clarify full vs half-bandwidth and defaults in time_frequency.multitaper.py (mne-tools#11479)
  Fix typo in tutorial (mne-tools#11498)
  Typo fix and added colons before code (mne-tools#11496)
  [MRG] ENH/DOC: demo custom spectrum creation (mne-tools#11493)
  Accept only left-clicks for adding annotations (mne-tools#11491)
  [BUG, MRG] Fix pial surface loading, logging in locate_ieeg (mne-tools#11489)
  [ENH] Added unit_role to add non-breaking space between magnitude  and units (mne-tools#11469)
  MAINT: Fix CircleCI build (mne-tools#11488)
  [DOC] Updated decoding.SSD documentation and internal variable naming (mne-tools#11475)
  Typo fix (mne-tools#11485)
  [MRG] Forward argument axes from plot_sensors to DigMontage.plot (mne-tools#11470)
  [MRG] Improve error message raised on channels missing from DigMontage (mne-tools#11472)
  MAINT: Deal with pkg_resources usage bugs (mne-tools#11478)
  Add object array support and docstring (mne-tools#11465)
  [ENH] Adjusted SSD algorithm to support non-full rank data (mne-tools#11458)
  [BUG] fix nibabel reference (mne-tools#11467)
  ...
@larsoner larsoner changed the title WIP updates for MPL 3.7 compatibility BUG: updates for MPL 3.7 compatibility Feb 22, 2023
@larsoner larsoner added backport-candidate on-merge: backport to maint/1.7 and removed needs-discussion issues requiring a dev meeting discussion before the way forward is clear labels Feb 22, 2023
@larsoner
Copy link
Member

Okay with my latest commit I get:

Code
import mne
raw = mne.io.read_raw_fif(mne.datasets.sample.data_path() / 'MEG' / 'sample' / 'sample_audvis_raw.fif')
raw.set_annotations(mne.Annotations([1], [2], ['test']))
with mne.viz.use_browser_backend('matplotlib'):
    raw.plot()
# Then do some manual clicking to make a new category + annotation
3.7 3.6
Screenshot from 2023-02-22 15-48-34 Screenshot from 2023-02-22 16-04-10

The spacing and stuff is perhaps not ideal and I had to use a private matplotlib object (argh) but it seems to work. @cbrnr can you try it? If so we can merge, backport, and cut a patch release

@tacaswell
Copy link

Can you open an issue that you need a way to get the colors as well? We only added API to set the colors assuming you were keeping track of that state someplace else anyway.

@larsoner
Copy link
Member

Can you open an issue that you need a way to get the colors as well? We only added API to set the colors assuming you were keeping track of that state someplace else anyway.

Actually it looks like we are already doing that, so I can just use our internal list of colors!

@cbrnr
Copy link
Contributor

cbrnr commented Feb 23, 2023

@larsoner it's not quite there, I noticed two issues:

  1. There are tick labels when there are no annotations:
    1
    Also, the "show/hide" label should also be hidden in this case.
  2. The left border of the checkboxes are missing:
    2

Other issues that are not that urgent (so could be fixed later):

  1. As you mentioned, the spacing is not ideal. It would be nice to make it look like the old version (e.g. radiobuttons should be left-aligned, the "show/hide" label should align with the checkboxes, etc.).
  2. When entering a label which already exists, a gray bar on top appears with no text. I'm not sure if this was different in the old version. I expected a text like "Duplicate label" appear.

@larsoner
Copy link
Member

I will probably just fix (1) here, leaving aesthetics (including 2) for later

@cbrnr
Copy link
Contributor

cbrnr commented Feb 23, 2023

Don't forget to turn on all tests that are expected to fail with MPL 3.7.

@larsoner larsoner enabled auto-merge (squash) February 23, 2023 19:12
@larsoner
Copy link
Member

Okay I think I fixed at least some of the bugs above, the bugs for CircleCI builds (I touched the failing examples here) and tested again on 3.7 and 3.6. I'll mark for merge when green and backport once it goes in!

@larsoner larsoner merged commit a5fa4ab into mne-tools:main Feb 23, 2023
larsoner added a commit that referenced this pull request Feb 23, 2023
@cbrnr
Copy link
Contributor

cbrnr commented Feb 23, 2023

Thanks a bunch @larsoner!

@drammock drammock deleted the test-mpl-fix branch February 24, 2023 15:36
@drammock drammock added backported and removed backport-candidate on-merge: backport to maint/1.7 labels Feb 24, 2023
larsoner added a commit to larsoner/mne-python that referenced this pull request Mar 1, 2023
* upstream/main: (264 commits)
  BUG: Fix deprecated API usage in example (mne-tools#11512)
  Deprecate 'kind' and 'path' in favor of 'fname' in the layout reader (mne-tools#11500)
  EGI/MFF events outside EEG recording should not break the code (mne-tools#11505)
  fixed annotations error on export (mne-tools#11435)
  DOC: Update installer links [skip azp] [skip actions] [skip cirrus] (mne-tools#11506)
  BUG: updates for MPL 3.7 compatibility (mne-tools#11409)
  Fix docstrings by replacing str with path-like and fix double backticks for formatting (mne-tools#11499)
  Use pathlib.Path instead of os.path to handle files and folders [circle deploy] (mne-tools#11473)
  MAINT: Fix Circle [circle deploy] (mne-tools#11497)
  MAINT: Use mamba in CIs (mne-tools#11471)
  Updating documentation to clarify full vs half-bandwidth and defaults in time_frequency.multitaper.py (mne-tools#11479)
  Fix typo in tutorial (mne-tools#11498)
  Typo fix and added colons before code (mne-tools#11496)
  [MRG] ENH/DOC: demo custom spectrum creation (mne-tools#11493)
  Accept only left-clicks for adding annotations (mne-tools#11491)
  [BUG, MRG] Fix pial surface loading, logging in locate_ieeg (mne-tools#11489)
  [ENH] Added unit_role to add non-breaking space between magnitude  and units (mne-tools#11469)
  MAINT: Fix CircleCI build (mne-tools#11488)
  [DOC] Updated decoding.SSD documentation and internal variable naming (mne-tools#11475)
  Typo fix (mne-tools#11485)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MAINT: Matplotlib deprecation of buttons.circles
4 participants