search

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
51 stars 50 forks source link

[MRG] Added plot sets functionality #746

Closed kmilo9999 closed 3 months ago

kmilo9999 commented 3 months ago
kmilo9999 commented 3 months ago

@gtdang give it a quick review and let me know your thoughts

codecov-commenter commented 3 months ago

Codecov Report

Attention: Patch coverage is 87.50000% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 92.62%. Comparing base (d28ddb7) to head (e9ec917). Report is 2 commits behind head on master.

:exclamation: Current head e9ec917 differs from pull request most recent head 327a3a6. Consider uploading reports for the commit 327a3a6 to get more accurate results

Files Patch % Lines
hnn_core/gui/_viz_manager.py 86.84% 5 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 #746 +/- ## ========================================== - Coverage 92.67% 92.62% -0.06% ========================================== Files 27 27 Lines 4928 4961 +33 ========================================== + Hits 4567 4595 +28 - Misses 361 366 +5 ```

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

jasmainak commented 3 months ago

@gtdang I let you iterate with @kmilo9999 and merge when happy!

Don't forget to update whats_new and would recommend keeping the commit history clean (< 10 commits)

kmilo9999 commented 3 months ago

@gtdang @ntolley Please review the test I included in test_gui.py about checking the creation of plots with using the new templates.

kmilo9999 commented 3 months ago

Awesome. Let me clean the commit history before merging with main

kmilo9999 commented 3 months ago

@gtdang This one is ready to go! 👍

kmilo9999 commented 3 months ago

@gtdang I added the Dipole-Spike plot. Review it if you want. It is ready to go.

gtdang commented 3 months ago

@ntolley This is ready to merge if you're OK with it.

ntolley commented 3 months ago

@kmilo9999 it seems like there are rebase conflicts? Happy to merge once those are fixed (i.e. running git rebase master on the most recent version of the master branch, and fixing the merge conflicts).

We can discuss in our Thursday meeting if there's too many conflicts

kmilo9999 commented 3 months ago

@kmilo9999 it seems like there are rebase conflicts? Happy to merge once those are fixed (i.e. running git rebase master on the most recent version of the master branch, and fixing the merge conflicts).

We can discuss in our Thursday meeting if there's too many conflicts

It was a small conflict in the whats_new.rst file. I resolved and it should be ready to merge after checks.

rythorpe commented 3 months ago

Something weird happened in your rebase @kmilo9999 . It appears that 501d82b got added to your commit history, which is from a previous PR.

jasmainak commented 3 months ago

@kmilo9999 please fetch master branch again and rebase ... it will get rid of the weird commits

kmilo9999 commented 3 months ago

@ntolley It should be ready to merge.