MRG: optimization updates after hnn-core integration #282
Conversation
blakecaldwell
force-pushed
the
hnn_core_optimizaton
branch
from
2d62bcd
to
6b8ee2f
Compare
blakecaldwell
changed the title
|
|
||
| sorted_inputs = dict(sorted(times_dict.items(), key=lambda item: item[1])) | ||
| for input_str in sorted_inputs.keys(): | ||
| input_list.append(input_str) |
There was a problem hiding this comment.
Not sure if this is a realistic failure point, but perhaps a test to ensure the numbering is sequential (t_evprox_1 -> t_exprox5) for example. Something like assert np.all(np.diff(ev_index_list) == 1))
There was a problem hiding this comment.
Yes, very reasonable... that is the check that would have alerted me to the issue addressed by commit 35352a4. I just won't have the time to implement CI checks for HNN GUI.
|
I'm good with this PR, which should wrap up the code changes for the hnn-core integration. Documentation PRs will follow and bug fixes as revealed by testing. |
|
Following the optimization tutorial produces a good fit. Slightly higher RMSE, but there are many reasons for that. This is after 2 optimization passes starting from ERPYes3Trials fit to S1_SupraT.txt |
| record_vsoma=record_vsoma) | ||
| record_vsoma = bool(net.params['record_vsoma']) | ||
|
|
||
| numspikes_params = net.params['numspikes_*'] |
There was a problem hiding this comment.
@jasmainak was it settled that it's ok to pass >2 spikes during the meeting? (as long as the tutorial is updated)
| sim_params = hnn_core_compat_params(self.params) | ||
| if sim_length is not None: | ||
| sim_params['tstop'] = sim_length | ||
| sim_params['tstop'] = round(sim_length, 8) |
There was a problem hiding this comment.
It's not clear to me why the rounding is necessary? I saw in the old commit that it is to avoid "precision errors"
There was a problem hiding this comment.
yeah hnn-core fails if tstop is 170.0000000002. the error is in adding vectors of different length for summing up dipole contributions
|
|
||
| # make copy of params dict in Params object before | ||
| # modifying tstop |
There was a problem hiding this comment.
| # modifying tstop | |
| # modifying tstop (necessary to avoid precision errors) |
There was a problem hiding this comment.
not quite the reason for copying params. tstop here can be reduced to run a shortened simulation (i.e. 110 ms).
hnn/simdata.py
Outdated
| @@ -231,7 +231,7 @@ class SimData(object): | |||
|
|
|||
| def __init__(self): | |||
| self._sim_data = {} | |||
| self._opt_data = {} | |||
| self._opt_data = {'initial_dpl': None, 'initial_error': 1e9} | |||
There was a problem hiding this comment.
Is this a placeholder or is there a reason for using 1e9? If not I would just got with float
There was a problem hiding this comment.
The key 'initial_error' just needed to exist. 1e9 is used elsewhere as an initial error (subsequent RMSE will always be lower)
Depends on terminate function in hnn-core 76162b10 Update exception handling for threads with a custom excepthook that doesn't emit an extra done signal. Also protect SimThread.killed variable with a lock because the main GUI thread accesses it via the stop method.
flake8 on qt_evoked.py
Caused range sliders not to work after merging code from hnn-core integration.
Correctly save initial dipole and initial error in SimData._opt_data dict so that plot_dipole can find it.
blakecaldwell
force-pushed
the
hnn_core_optimizaton
branch
from
e7b6f73
to
faf56c0
Compare
|
Thanks for the review @ntolley. Rebased and addressed comments. Please merge if OK |
|
Thanks @blakecaldwell! I'll dig into the GUI code more as we move forward. |
This PR is about fixing the remaining bugs in the optimization code and "reproducing" the optimization tutorial.