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

[ENH] Added tonic input to GUI drivers tab #773

Closed kmilo9999 closed 1 month ago

kmilo9999 commented 1 month ago

Wok on new Feature to add tonic input from in the GUI Solves #715

image image

gtdang commented 1 month ago

@ntolley @jasmainak @rythorpe Thoughts on if times should be above or below the amplitudes? Also the Synchronous Inputs checkbox isn't needed for Tonics, correct?

rythorpe commented 1 month ago

@ntolley @jasmainak @rythorpe Thoughts on if times should be above or below the amplitudes?

I vote above, but it's a mild preference.

rythorpe commented 1 month ago

Correct, synchronous inputs don't make sense to tonic inputs.

codecov-commenter commented 1 month ago

Codecov Report

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

Project coverage is 92.28%. Comparing base (819f4f4) to head (4bb3f23). Report is 32 commits behind head on master.

:exclamation: Current head 4bb3f23 differs from pull request most recent head 301948d

Please upload reports for the commit 301948d to get more accurate results.

Files Patch % Lines
hnn_core/gui/gui.py 78.84% 11 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 #773 +/- ## ========================================== - Coverage 92.33% 92.28% -0.05% ========================================== Files 27 27 Lines 5059 5096 +37 ========================================== + Hits 4671 4703 +32 - Misses 388 393 +5 ```

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

kmilo9999 commented 1 month ago

This is the new order of the amplitudes image

gtdang commented 1 month ago

I played around with this and replicated the Simulate Gamma API Tutorial tonic example. Below is the plot comparing the simulations with and without the added tonic. Screenshot 2024-05-22 at 11 09 01 AM I think everything is working as expected! Great job Camilo!

Some quality of life considerations.

  • When adding a tonic drive, the default Stop Time is set to 0. Should this sync with the simulation tstop parameter on creation?
  • The simulation tab stop time label is "tstop (ms)" while in the tonic it's "Stop Time". Should we be consistent here? And which do you prefer?
  • I think the cell types should be ordered alphabetically: [L2_basket, L2_pyramidal, L5_basket, L5_pyramidal]

@ntolley @jasmainak @rythorpe Thoughts?

This was discussed at our dev sync. 1.) We are not syncing tstop. Poisson drive does not. 2.) Will open an issue to standardize name and unit labels 3.) Cell types are ordered alphabetically now.

kmilo9999 commented 1 month ago

@gtdang @ntolley The CodeSpell checks fails in a file I have not modified in this PR

gtdang commented 1 month ago

Great work @kmilo9999 !!

I'm a bit confused about the codespell, I wonder if the github action somehow got updated and is now checking new files...

Do you think you could modify the typos in any case? It's just the 3 lines and they'll need to be fixed at some point

Created a separate PR for those typos #777

ntolley commented 1 month ago

@gtdang is this request satisfied?

  • [ ] Add test to check tonic widget is created with prespecified_drive_data

Did you mean for there to be a test with assert statements?

kmilo9999 commented 1 month ago

@gtdang is this request satisfied?

  • [ ] Add test to check tonic widget is created with prespecified_drive_data

Did you mean for there to be a test with assert statements?

This prespecified_drive_data function reads the params from default.json, and I dont see a tonic bias input described on this file. Also, I think some logic needs to be implemented to be able to read and load tonic bias from a file.

gtdang commented 1 month ago

@gtdang is this request satisfied?

  • [ ] Add test to check tonic widget is created with prespecified_drive_data

Did you mean for there to be a test with assert statements?

This prespecified_drive_data function reads the params from default.json, and I dont see a tonic bias input described on this file. Also, I think some logic needs to be implemented to be able to read and load tonic bias from a file.

@ntolley is there an example of the flat a json or param file with tonics encoded?

ntolley commented 1 month ago

These are the parameters in the .param file that need to be modified image

Do you think we should save loading for a separate PR and go ahead with merging this one?

kmilo9999 commented 1 month ago

These are the parameters in the .param file that need to be modified image

Do you think we should save loading for a separate PR and go ahead with merging this one?

@ntolley why the L2Basketand L5Basketcell types dont have the suffix _soma?

ntolley commented 1 month ago

@ntolley why the L2Basket and L5Basket cell types dont have the suffix _soma?

These cells are modeled as just one single Section, so the whole cell is represented by the soma.

ntolley commented 1 month ago

Merging since every test is passing besides Ubuntu (which is in progress)

Thanks @kmilo9999!!!