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

[MRG] Added data check on axes in `test_gui.py` #726

Closed samadpls closed 6 months ago

samadpls commented 6 months ago

Fixes issue #697

jasmainak commented 6 months ago

@gtdang can you review?

codecov-commenter commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 89.44%. Comparing base (df99e07) to head (51b6ee5). Report is 1 commits behind head on master.

:exclamation: Current head 51b6ee5 differs from pull request most recent head 09d1e1e. Consider uploading reports for the commit 09d1e1e to get more accurate results

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

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #726 +/- ## ========================================== + Coverage 89.06% 89.44% +0.37% ========================================== Files 28 28 Lines 5068 5068 ========================================== + Hits 4514 4533 +19 + Misses 554 535 -19 ```

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

gtdang commented 6 months ago

This change is not needed as there is already a check for the default figure production in test_gui_add_figure.

The default figures are the figures that are automatically generated after running a simulation. The figures that need to be checked are the figures that the user can manually create using the Visualization tab. See image below. Screenshot 2024-03-12 at 10 33 34 AM

Of this list "current dipole" and "histogram" don't need checks as they are part of the default set of plots. The others will need to have unique test functions developed except for the spectrogram, which already has a unique test test_gui_adaptive_spectrogram. However the spectrogram does need to have a check for if data is plotted.

rythorpe commented 6 months ago

Also, @samadpls, if you use the word "fixes" or "closes" followed by the issue tag, GH will automatically detect it and close the associated issue when your PR gets merged. Just a fun trick! :smile:

samadpls commented 6 months ago

@gtdang, thank you for your suggestions. 🙌🏻 I have added the test cases for the mentioned plots above. Please let me know if there's anything else I need to address

gtdang commented 6 months ago

@jasmainak @dylansdaniels @ntolley Do we update the whats_new.rst for a change like this? Or only user-facing changes?

jasmainak commented 6 months ago

No updates to whats_new.rst ... it's only for user facing changes!

gtdang commented 6 months ago

Looks good except the last nitpick ... I'll let @gtdang hit the "Squash and merge" button if he is happy.

Great first contribution! Shoot us an email to start discussing the GSoC project. @ntolley is your point person for that.

Squash and not rebase?

jasmainak commented 6 months ago

We want to keep the commit history clean ... the diff should have been one or two commits. Rebase and merge is good when it's a bigger PR and we want to track the history of the changes.