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

kmilo9999 · 2024-05-20T14:20:44Z

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

gtdang · 2024-05-21T13:59:53Z

@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 · 2024-05-21T14:08:04Z

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

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

rythorpe · 2024-05-21T14:09:53Z

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

gtdang

Looking good Camilo! Per the discussion:

Remove Sync Inputs checkbox from tonics
Move times above amplitudes
Add test to check tonic widget is created with prespecified_drive_data
Add test to check an added tonic amplitude is added to the network when _init_network_from_widgets is called

gtdang · 2024-05-21T14:23:17Z

hnn_core/gui/gui.py

+                amplitude=weights_amplitudes,
+                t0=drive["t0"].value,
+                tstop=drive["tstop"].value)
+            return


Should this be returning?

The rest of the logic in that function doesnt apply to Tonic inputs. I want to avoid the elif branching

We talked it over and I'll remove the return

codecov-commenter · 2024-05-21T19:47:27Z

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.

❗ 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 ⚠️

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

Additional details and impacted files

@@            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

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

hnn_core/gui/gui.py

gtdang

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.

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?

kmilo9999 · 2024-05-23T13:17:35Z

This is the new order of the amplitudes

ntolley · 2024-05-23T18:54:24Z

hnn_core/gui/gui.py

@@ -457,6 +458,10 @@ def _simulation_list_change(value):
                    is_disabled="", btn_height=self.layout['run_btn'].height,
                    color_theme=self.layout['theme_color']))

+        def _driver_type_change(value):
+            self.widget_location_selection.disabled = (


Very useful! Glad to see this is implemented natively

ntolley · 2024-05-23T18:57:16Z

hnn_core/gui/gui.py

+        'amplitude': 0,
+        't0': 0,
+        'tstop': 0,
+        'seedcore': 14,


Suggested change

'seedcore': 14,

is seedcore actually used anywhere? net.add_tonic_drives doesn't have any randomization so probably safe to delete

ntolley · 2024-05-23T19:02:11Z

hnn_core/gui/gui.py

+
+    start_times = BoundedFloatText(
+        value=deepcopy(default_data['t0']), description="Start time",
+        min=0, max=1e6, step=0.01, **kwargs)


Suggested change

min=0, max=1e6, step=0.01, **kwargs)

min=0, max=1e6, step=1.0, **kwargs)

This is a pretty small step size considering what's being changed

ntolley · 2024-05-23T19:02:28Z

hnn_core/gui/gui.py

+        min=0, max=1e6, step=0.01, **kwargs)
+    stop_times = BoundedFloatText(
+        value=deepcopy(default_data['tstop']), description="Stop time",
+        min=-1, max=1e6, step=0.01, **kwargs)


Suggested change

min=-1, max=1e6, step=0.01, **kwargs)

min=-1, max=1e6, step=1.0, **kwargs)

ntolley · 2024-05-23T19:04:55Z

hnn_core/gui/gui.py

@@ -1026,6 +1082,10 @@ def add_drive_widget(drive_type, drive_boxes, drive_widgets, drives_out,
                     sync_evinput=False):
    """Add a widget for a new drive."""

+    # Check only adds 1 tonic input widget
+    if drive_type == "Tonic" and not _is_valid_add_tonic_input(drive_widgets):
+        return


Does this just mean the add drive button doesn't do anything once you add a tonic drive?

That is correct.
@gtdang

gtdang · 2024-05-23T19:06:08Z

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. 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 · 2024-05-23T20:26:52Z

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

ntolley

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

gtdang · 2024-05-24T15:40:39Z

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 · 2024-05-25T21:08:37Z

@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 · 2024-05-26T16:08:55Z

@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 · 2024-05-29T13:52:35Z

@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 · 2024-05-29T14:33:42Z

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

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

kmilo9999 · 2024-05-29T20:41:21Z

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

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

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

ntolley · 2024-05-30T01:15:37Z

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

moving to another PR based on discussion

ntolley · 2024-06-06T18:57:32Z

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

Thanks @kmilo9999!!!

kmilo9999 changed the title ~~[ENH] Added tonic input to GUI drivers tab~~ [WIP] Added tonic input to GUI drivers tab May 20, 2024

kmilo9999 linked an issue May 21, 2024 that may be closed by this pull request

GUI tonic inputs #715

Closed

3 tasks

kmilo9999 requested review from gtdang and ntolley May 21, 2024 02:53

kmilo9999 changed the title ~~[WIP] Added tonic input to GUI drivers tab~~ [ENH] Added tonic input to GUI drivers tab May 21, 2024

kmilo9999 marked this pull request as ready for review May 21, 2024 02:56

gtdang previously requested changes May 21, 2024

View reviewed changes

kmilo9999 requested a review from gtdang May 21, 2024 19:36

gtdang reviewed May 22, 2024

View reviewed changes

hnn_core/gui/gui.py Outdated Show resolved Hide resolved

kmilo9999 requested a review from gtdang May 22, 2024 14:30

gtdang reviewed May 22, 2024

View reviewed changes

kmilo9999 requested a review from gtdang May 22, 2024 17:50

kmilo9999 mentioned this pull request May 23, 2024

MAINT: Fixed load data button size #775

Merged

ntolley reviewed May 23, 2024

View reviewed changes

kmilo9999 requested a review from ntolley May 23, 2024 21:08

ntolley approved these changes May 23, 2024

View reviewed changes

kmilo9999 force-pushed the feat-gui-tonic-bias branch from 4e82443 to bb0fc1a Compare May 24, 2024 14:56

kmilo9999 and others added 9 commits May 26, 2024 11:32

ENH: Added tonic input to GUI drivers tab

210f20a

MAINT: Finished work on Tonic bias widget

71208d0

MAINT: Moving times to the top. Fixing logic adding Tonic input

1314568

TST: Added tonic input widget test

0340b3a

MAINT: Removing debug code

649c90a

MAINT: Validate only 1 tonic input widget is added

cc74b8c

MAINT: Changed tonic amplitude keys order

2952e13

MAINT: Chainging time steps. Removing seedore from tonic input bias

c2488d4

DOC: Adding feature entry in whats_new.rst

301948d

kmilo9999 force-pushed the feat-gui-tonic-bias branch from bb0fc1a to 301948d Compare May 26, 2024 15:33

ntolley merged commit 86b971a into jonescompneurolab:master Jun 6, 2024
13 of 17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kmilo9999 commented May 20, 2024 •

edited

gtdang commented May 21, 2024 •

edited

rythorpe commented May 21, 2024

rythorpe commented May 21, 2024

gtdang left a comment •

edited by kmilo9999

gtdang May 21, 2024

kmilo9999 May 21, 2024

kmilo9999 May 21, 2024

codecov-commenter commented May 21, 2024 •

edited

gtdang left a comment

kmilo9999 commented May 23, 2024 •

edited

ntolley May 23, 2024

ntolley May 23, 2024

kmilo9999 May 23, 2024

ntolley May 23, 2024

kmilo9999 May 23, 2024

ntolley May 23, 2024

kmilo9999 May 23, 2024

ntolley May 23, 2024

kmilo9999 May 23, 2024

gtdang commented May 23, 2024

kmilo9999 commented May 23, 2024

ntolley left a comment

gtdang commented May 24, 2024

ntolley commented May 25, 2024

kmilo9999 commented May 26, 2024

gtdang commented May 29, 2024

ntolley commented May 29, 2024

kmilo9999 commented May 29, 2024 •

edited

ntolley commented May 30, 2024

ntolley commented Jun 6, 2024

	min=0, max=1e6, step=0.01, **kwargs)
	min=0, max=1e6, step=1.0, **kwargs)

	min=-1, max=1e6, step=0.01, **kwargs)
	min=-1, max=1e6, step=1.0, **kwargs)

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

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

Conversation

kmilo9999 commented May 20, 2024 • edited

gtdang commented May 21, 2024 • edited

rythorpe commented May 21, 2024

rythorpe commented May 21, 2024

gtdang left a comment • edited by kmilo9999

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented May 21, 2024 • edited

Codecov Report

gtdang left a comment

Choose a reason for hiding this comment

kmilo9999 commented May 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gtdang commented May 23, 2024

kmilo9999 commented May 23, 2024

ntolley left a comment

Choose a reason for hiding this comment

gtdang commented May 24, 2024

ntolley commented May 25, 2024

kmilo9999 commented May 26, 2024

gtdang commented May 29, 2024

ntolley commented May 29, 2024

kmilo9999 commented May 29, 2024 • edited

ntolley commented May 30, 2024

ntolley commented Jun 6, 2024

kmilo9999 commented May 20, 2024 •

edited

gtdang commented May 21, 2024 •

edited

gtdang left a comment •

edited by kmilo9999

codecov-commenter commented May 21, 2024 •

edited

kmilo9999 commented May 23, 2024 •

edited

kmilo9999 commented May 29, 2024 •

edited