amplitude based stimuli in apply_multiple_stimuli #182
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #182 +/- ##
==========================================
- Coverage 91.23% 91.18% -0.06%
==========================================
Files 95 95
Lines 5626 5662 +36
==========================================
+ Hits 5133 5163 +30
- Misses 493 499 +6 ☔ View full report in Codecov by Sentry. |
|
It looks like my additions to the tests increased the ci testing time. :') |
| @@ -32,16 +32,19 @@ def mock_run_stimulus(): | |||
| def test_apply_multiple_step_stimuli(mock_run_stimulus): | |||
| """Do not run the code in parallel, mock the return value via MockRecording.""" | |||
| amplitudes = [80, 100, 120, 140] | |||
| thres_perc = [0.08, 0.1, 0.12, 0.14] | |||
There was a problem hiding this comment.
For test time maybe instead of 4, just 1 or 2 values can be kept here
There was a problem hiding this comment.
In the future more protocols and therefore more tests will be added, so the tests will take even longer. Maybe after running the protocols once, the other tests can just mock the expensive neuron simulation run part to retrieve precomputed (or even constant) voltage results to us. That way the newly added code still can be tested.
| amplitude=amplitude, | ||
| ) | ||
|
|
||
| raise TypeError("You have to give either threshold_current or amplitude") |
There was a problem hiding this comment.
I like to catch these edge cases in the tests. It may look trivial but there are sometimes issues due to those.
| threshold_current: Optional[float] = None, | ||
| threshold_percentage: Optional[float] = 200.0, | ||
| amplitude: Optional[float] = None, |
There was a problem hiding this comment.
What if the user mistakenly provides all the 3 of threshold_current, threshold_percentage and amplitude?
Or what if both the amplitude and threshold_current are provided but the threshold_percentage missing?
Can those cases also be covered with logs/exceptions/warnings?
|
Great! This should solve Issue #180 |
can choose to use
apply_multiple_stimuliwith amplitude based stimuli by passing raw amplitudes toamplitudesand settingthreshold_basedtoFalse.