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
55 stars 52 forks source link

[MRG] GUI dipole plot bug. #695

Closed gtdang closed 10 months ago

gtdang commented 11 months ago

closes #694 #688

gtdang commented 11 months ago

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()
codecov-commenter commented 10 months ago

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%.

:exclamation: 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 :warning:

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

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jasmainak commented 10 months ago

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

gtdang commented 10 months ago

@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 commented 10 months ago

@jasmainak Added a check in the tests for data on the axis with https://github.com/jonescompneurolab/hnn-core/pull/695/commits/9d95b463665cb4e4f70c1a95905fffabe34e4514.

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

jasmainak commented 10 months ago

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 commented 10 months ago

@ntolley merge button is yours!