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

[MRG] GUI dipole plot bug. #695

Merged
merged 2 commits into from
Dec 11, 2023

Conversation

gtdang
Copy link
Collaborator

@gtdang gtdang commented Dec 8, 2023

  • Refactored GUI color cycling be compatible with matplotlib 3.8.x deprecation of ax._get_lines.prop_cycler. Substituted with ax._get_lines.get_next_color().
  • Tested with older versions matplotlib==3.5.3 and 3.7.4 to make sure the plots still work.

closes #694 #688

…lib 8.x deprecation of ax._get_lines.prop_cycler.
@gtdang
Copy link
Collaborator Author

gtdang commented Dec 8, 2023

Below is the code I used to run the gui to debug with an IDE. @ntolley @dylansdaniels Note that this is a much simpler way to do it without importing the underlying run function that I showed during our meeting.

from hnn_core.gui import HNNGUI

def test_debug_gui():
    gui = HNNGUI()
    gui.compose()
    gui.run_button.click()

@@ -150,7 +150,7 @@ def _update_ax(fig, ax, single_simulation, sim_name, plot_type, plot_config):

elif plot_type == 'PSD':
if len(dpls_copied) > 0:
color = next(ax._get_lines.prop_cycler)['color']
color = ax._get_lines.get_next_color()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this API change work for earlier versions of matplotlib as well? If so I'm definitely happy to merge as is!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I developed with 3.8.2 and tested it with matplotlib older versions 3.5.3 and 3.7.4. ">=3.5.3" is what is specified in the setup file.

@codecov-commenter
Copy link

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (1216038) 91.34% compared to head (0c0599c) 91.34%.

❗ Current head 0c0599c differs from pull request most recent head d3ec97e. Consider uploading reports for the commit d3ec97e to get more accurate results

Files Patch % Lines
hnn_core/gui/_viz_manager.py 50.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #695   +/-   ##
=======================================
  Coverage   91.34%   91.34%           
=======================================
  Files          25       25           
  Lines        4599     4599           
=======================================
  Hits         4201     4201           
  Misses        398      398           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jasmainak
Copy link
Collaborator

@gtdang any idea why was this problem not caught in the tests so far?

@gtdang
Copy link
Collaborator Author

gtdang commented Dec 11, 2023

@gtdang any idea why was this problem not caught in the tests so far?

From quickly looking at the tests... This error wouldn't have be caught by test_dipole.py because the error wasn't occurring in the plotting function, but just right before it within the GUI's viz_manager _update_ax function.

test_gui.py doesn't catch the error because there's not checks on the actual figure attributes, only on the ipywidget api elements. In this case the figure tab & output widgets are generated and even the matplotlib axis in the outputs widgets are generated, but nothing was being plotted on the axis. I suppose a check that makes sure the figure axis are not empty would have flagged this error.

Oh, also do we know which version(s) of matplotlib the automated tests are running? If they are running <3.8 then there would be no problem with the GUI...

@gtdang
Copy link
Collaborator Author

gtdang commented Dec 11, 2023

@jasmainak
Added a check in the tests for data on the axis with 9d95b46.

We should probably add checks for each type of figure that can be generated through the GUI in a follow-up PR.

@jasmainak
Copy link
Collaborator

Beautiful, good to go once tests pass. @gtdang once it's ready on your end, please set the title to be [MRG]. We use WIP and MRG to indicate the state of the PR ... prevents us from merging a PR that's not ready yet

@jasmainak
Copy link
Collaborator

@ntolley merge button is yours!

@gtdang gtdang changed the title fix: GUI dipole plot bug. [MRG] GUI dipole plot bug. Dec 11, 2023
assert gui.viz_manager.fig_idx['idx'] == 2

# Check default figs have data on their axis
assert gui.viz_manager.figs[1].axes[0].has_data()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the has_data() docstring:

This should not be used to determine whether the dataLim need to be updated, and may not actually be useful for anything.

Useful for us! 😄

@ntolley ntolley merged commit 34d972c into jonescompneurolab:master Dec 11, 2023
9 checks passed
@gtdang gtdang mentioned this pull request Jan 16, 2024
1 task
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.

[BUG] plot_dipole not showing in GUI with matplotlib>=3.8.0
4 participants