Conversation
clinssen
left a comment
clinssen
left a comment
There was a problem hiding this comment.
Thanks for fixing this bug and adding the FitzHugh-Nagumo model! I left several comments, hopefully most of them should be relatively minor changes.
Could you please file a separate PR containing the typing bugfix? Then we'll try to get that merged into master first.
odetoolbox/integrator.py
Outdated
| :param spike_times: For each variable, used as a key, the list of spike times associated with it. | ||
| """ | ||
|
|
||
There was a problem hiding this comment.
Remove unnecessary whitelines.
tests/test_fitzhughnagumo.py
Outdated
| @@ -0,0 +1,164 @@ | |||
| # | |||
| # test_mixed_integrator_numeric.py | |||
tests/fitzhughnagumo.json
Outdated
|
|
||
| "options": { | ||
| "sim_time": "45E-3", | ||
| "integration_accuracy_abs" : "1E-6", |
There was a problem hiding this comment.
Perhaps the entire "options" block can be removed. Could you double-check that the default integration accuracy is adequate (i.e. results look the same when we remove this block from the model)?
There was a problem hiding this comment.
Caveat: note that the integration accuracy, passed here to ODE-toolbox as input, will be used by ODE-toolbox during its analysis, and is (possibly) a different value from the one used during the numeric integration in the unit test.
tests/test_fitzhughnagumo.py
Outdated
|
|
||
|
|
||
| class TestFitxhughNagumo(unittest.TestCase): | ||
|
|
tests/test_fitzhughnagumo.py
Outdated
| peaks, _ = find_peaks(np.array(y_log)[N1:,0], height = 1.5 ) #finding peaks above 1.5 microvolts ignoring the first 200 ms | ||
| num_peaks[j] = (int)(len(peaks)/((T-200)*0.001)) #frequency (in Hz) of the peaks for every value of current | ||
| if(I_ext[j] >(1/3)): | ||
| assert(num_peaks[j]>20) |
There was a problem hiding this comment.
Parentheses not necessary.
tests/test_fitzhughnagumo.py
Outdated
| if INTEGRATION_TEST_DEBUG_PLOTS: | ||
| self._FI_curve(I_ext,num_peaks,basedir="",fn_snip = "FI curve", title_snip = "FI curve") | ||
|
|
||
| def _timeseries_plot(self,N1, t_log, h_log, y_log, sym_list, basedir="", fn_snip="", title_snip=""): |
There was a problem hiding this comment.
Explain what N1 is here in the docstring.
tests/test_fitzhughnagumo.py
Outdated
|
|
||
| h = 1 # [ms] #time steps | ||
| T = 1000 # [ms] #total simulation time | ||
| n = 25 #total number of current values between 0 and 1 |
There was a problem hiding this comment.
I would suggest to drastically lower this number if we're not making any plots, just so the testsuite doesn't take too long to complete on Travis. Possibly even a manually curated list (I_ext = np.array([0., ..., 1.]))
tests/test_fitzhughnagumo.py
Outdated
| plt.savefig(fn,dpi=600) | ||
| plt.close() | ||
|
|
||
|
|
tests/test_fitzhughnagumo.py
Outdated
| shapes, | ||
| analytic_solver_dict=None, | ||
| parameters={"I_ext":str(I_ext[j])}, | ||
| random_seed=123, |
There was a problem hiding this comment.
| random_seed=123, |
Not important for this test.
|
Thanks for the updates addressing my comments. I still have a few remaining concerns. I'm still not happy with the time it takes the unit testing suite to complete. I would suggest to move to a manually curated list: I_ext_list = np.array([1/3 - .1, 1/3 + .1])and change the loop from n = 10
for j in range(n):to for j, I_ext in enumerate(I_ext_list):The test if I_ext[j] > 1 / 3:has to be changed to include a margin. After all, because of floating point discretisation errors (the minutiae of which might even depend on the model of CPU that we're running on), we can't really predict very well what happens near I_ext = 1/3. So would need something like: if I_ext < 1/3 - margin:
# assert NO spikes
elif I_ext > 1/3 + margin:
# assert spiking
# else
# # don't know what might happen!Please include a comment in the code (where appropriate) to note that the current I_ext should be in the range [0, 1] (inclusive). Could you also fix the merge conflict, by pulling latest upstream/master and picking the line without the comment? Cheers! |
2 participants
first version of fitzhughnagumo