jonescompneurolab / hnn-core

Simulation and optimization of neural circuits for MEG/EEG source estimates
https://jonescompneurolab.github.io/hnn-core/
BSD 3-Clause "New" or "Revised" License
53 stars 52 forks source link

[BUG] `plot_dipole` not showing in GUI with `matplotlib>=3.8.0` #694

Closed ntolley closed 8 months ago

ntolley commented 9 months ago

Reproduced by creating a new conda environment and doing a fresh pip install hnn_core[gui]. Simulation outputs will look like: image

Normal functionality is restored by downgrading matplotlib with pip install matplotlib==3.7.3

@rythorpe @jasmainak this might have to do with #586 which really should just be a temporary fix and could be revisited

rythorpe commented 9 months ago

Ugh. So maybe us pinning the voila dependency requirement to an earlier version is coming back to haunt us?

jasmainak commented 9 months ago

I noticed as well. We really shouldn't be pinning versions. Perhaps a good starting issue for the ccv folks

gtdang commented 9 months ago

Is this the same issue as #688 or is that a different problem? Agree that pinning versions should be a last resort.

jasmainak commented 9 months ago

The same very likely! @gtdang jupyter widgets made an API breaking change with the 8.0 release. See here: https://ipywidgets.readthedocs.io/en/latest/user_migration_guides.html

We need to migrate our code to this version. It's a good opportunity to read the GUI code and understanding its structure.

gtdang commented 9 months ago

So I started poking into this issue as well as the pinning of the voila/ipywidgets issue.

Good news: Using the latest versions of ipywidgets and voila I was able to get the widget up and running.

Bad news: Default plots are not displaying at the end of simulation.

Consolation: The plots generate when you manually create a figure, including the dipole plots!

Environment: Python 3.9.16 ipywidgets 8.1.1, ipympl 0.9.3, voila 0.5.5, matplotlib 3.8.2 3.7.4

There was no javascript error like in #585 #586 but rather an error in title assignment of the accordion widget due to the API breaking changes @jasmainak mentioned. This minor fix allows the widget to launch without error.

Issue is that the plots don't display automatically after the simulation concludes. Screenshot 2023-12-04 at 3 53 44 PM

However if you manually generate the visualizations they work. Even the problematic dipole plots. And this is using the latest version of matplotlib. [Sorry this was actually using V7.7.4]

Screenshot 2023-12-04 at 3 54 24 PM

Since the plots do show up with manual generation, the issue with the plots not showing up after simulation is probably just some issue with a callback function at simulation conclusion. I'll try to figure that out tomorrow and get these changes into a PR.

gtdang commented 9 months ago

I was mistaken previously and was using Matplotlib v7.7.4 and so the dipole plots worked. I started looking into the problem again and found the source of the issue to be the prop_cycler iterator being removed from ax._get_lines in Matplotlib 8.x mpl Issue 26831. We use this in the _viz_manager module in _update_ax function for the PSD and Dipole plots to cycle to the next line color. I'm assuming this important for when the axis is called on again to add another simulation.

The issue is not something matplotlib is intending to fix so we'll have to look into a work-around.

jasmainak commented 9 months ago

Great sluething @gtdang ! what would you recommend as a workaround for the prop_cycler? Could we pre-define a color cycler? As in : https://matplotlib.org/stable/users/explain/artists/color_cycle.html

gtdang commented 9 months ago

@jasmainak there's a default color cycler that's already set by rcParams, so we don't need to pre-define one unless we want to use a different set of colors from the defaults. Previously we were using the prop_cycler to iterate through the default cycler. This is important when adding multiple simulations to the same plot, so that each simulation will plot with a different color.

It seems like color = ax._get_lines.get_next_color() does the same thing and works for mpl 8.x. I'm not sure when this method was introduced, so we'll have to test if this change will break matplotlib <8.x.

ntolley commented 9 months ago

If we feel the breaking matplotlib version is too recent I suppose one option is to add logic to detect the version...