[MRG] Add tonic inputs #635
Conversation
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## master #635 +/- ##
==========================================
- Coverage 92.19% 91.23% -0.97%
==========================================
Files 22 22
Lines 4282 4426 +144
==========================================
+ Hits 3948 4038 +90
- Misses 334 388 +54
|
|
@chenghuzi I think a tonic drive is getting added that should be skipped: Do you think you could follow the logic in #619 to filter out invalid tonic drives? I think the culprit function is |
sure, but I guess it's |
It should be fixed now. I didn’t open a new PR as it’s a minor modification, and adding just a filter condition in |
| if drive_type in [ | ||
| 'Evoked', 'Poisson', 'Rhythmic', 'Bursty', 'Gaussian' | ||
| 'Evoked', 'Poisson', 'Rhythmic', 'Bursty', 'Gaussian', 'Tonic' |
There was a problem hiding this comment.
Probably not related to this PR, but do we still need references to the 'Gaussian' drive type anywhere in the GUI?
There was a problem hiding this comment.
There was a problem hiding this comment.
humm ... but I don't get how you can have a Gaussian drive type. It's not an option in the GUI
| start_times[cell_type] = BoundedFloatText( | ||
| value=copy.deepcopy(default_data['t0']), description=cell_type, | ||
| min=0, max=1e6, step=0.01, **kwargs) | ||
| stop_times[cell_type] = BoundedFloatText( | ||
| value=copy.deepcopy(default_data['tstop']), description=cell_type, | ||
| min=-1, max=1e6, step=0.01, **kwargs) |
There was a problem hiding this comment.
Is there a reason why we set independent start/stop times for each cell type?
There was a problem hiding this comment.
you mean the range? or the data values?
There was a problem hiding this comment.
Like, I can currently add a single tonic bias that starts and stops at different times depending on the cell type. If someone wants to set different dynamics for different cell types, they should just create separate baises that correspond one-to-one with the different cell types.
There was a problem hiding this comment.
agree about that ... the GUI should reflect what the hnn-core API says, which is one cell-type per bias
There was a problem hiding this comment.
Wait, a bias is only supposed to target a single cell type?
| cell_types = list(drive['amplitude'].keys()) | ||
| assert set(cell_types) == set(drive['t0'].keys()) == set( | ||
| drive['tstop'].keys()) | ||
| for cell_type in cell_types: |
There was a problem hiding this comment.
as we discussed above, I think we don't need this external loop ... it will greatly simplify your code and the interface. Just a dropdown for selecting the cell_type is needed
There was a problem hiding this comment.
I kind of forgot the details. could you correct me if I am wrong? are you saying that the bias has the same value for all cell types? And thus we don't need to let the user to set different values for different cell types
There was a problem hiding this comment.
I think we decided that the start and stop times should be the same for all cell types targetted by the tonic drive, but that this needs to be changed at the API-level within Network.add_tonic_bias().
There was a problem hiding this comment.
For now I think we can simply link those components so that they look all the same. This is the most straightforward solution and visually reinforces to the user they are meant to be the same, rather than implying an error on our end.
There was a problem hiding this comment.
Check the latest commit, we just linked them alltogether
There was a problem hiding this comment.
It should probably be it's own PR but shouldn't take too long. If I get a chance this week, I'm happy to open a PR.
There was a problem hiding this comment.
I agree with Ryan that we should add an option to specify the tonic biases using a dictionary so that the logic of network instantiating is not in the GUI. The GUI should be a thin layer on top of the computational core
There was a problem hiding this comment.
It should probably be it's own PR but shouldn't take too long. If I get a chance this week, I'm happy to open a PR.
@rythorpe Was this API fix ever worked on? What is the proposed change for the method? Is it to be able to apply the bias to several different cell types in one call?
There was a problem hiding this comment.
Unfortunately I never got around to this.
Yes, the idea was to be able to set which cell types are targeted in Network.add_tonic_bias() (similar to the way we set which cells are targeted by setting weights in Network.add_evoked_drive()). The best way to do with would probably be to remove the cell_type arg and modify the amplitude arg to be dict that maps amplitude to a given cell type key.
There was a problem hiding this comment.
we should certainly fix the API before adding to the GUI
As titled.