MRG: make joblib work #44
Conversation
Codecov Report
@@ Coverage Diff @@
## master #44 +/- ##
==========================================
- Coverage 81.87% 81.51% -0.36%
==========================================
Files 14 14
Lines 1401 1439 +38
==========================================
+ Hits 1147 1173 +26
- Misses 254 266 +12
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I would be suspicious of NEURON not behaving as you expect (idempotent network instantiation). Tests, as they are currently written, instantiate two Network objects in a single Python process (presumably a single NEURON context). Calling pc.gid_clear() was necessary to avoid a segfault.
32884ad
|
I think moving forward with an embarrassing parallel option (this PR) and an MPI option makes sense. I may be the only user using MPI option for a while, but it's how I plan on running optimization with ~600 cores. Dask distributed required development versions and communicates over TCP instead of InfiniBand on Oscar. So let's continue to look at how they can both be used rather than choosing one or the other. From our combined experience, we might be able to develop a scheme more suitable for the general user than what I'm using for the short-term research. |
Multiple trials FIX: use cleaner import for joblib ENH: change random seed for each trial
| The Network object specifying how cells are | ||
| connected. | ||
| if trial_idx != 0: | ||
| params['prng_*'] = trial_idx |
There was a problem hiding this comment.
@blakecaldwell does this seem legit to you? For simulating different trials, I just changed the random seed. The way the random seed is changed is different (and simpler) than in HNN -- so the results could be slightly different for n_trials > 1. But is there anything else that needs to change from one trial to the next?
Feel free to fetch this branch locally and try it. I'll wait for your feedback before merging it.
There was a problem hiding this comment.
These seem like unnecessary changes at this point. Simplifying schemes like the randomization of inputs is appealing to me, but I pragmatically believe that all code cleanup changes should not change the model output.
In PR #55 (which conflicts with this PR) I have already simplified this as far as possible without changing results.
9402e2c#diff-f4f8654c734567d73fec96e0e0dacce5R112
There was a problem hiding this comment.
sure, although I don't quite understand what is different in the HNN code. It seems to be essentially setting a bunch of seeds -- unless I am missing something? Now you could argue that the particular seeds used by the HNN code are important so that we can compare things. In that case, I think we should set up a test first. Save data with 2 or more trials from HNN and compare against mne-neuron code in the test. And then make changes so that the test passes. That way, I am not afraid of making changes which will potentially break things ...
There was a problem hiding this comment.
I fully support a test to compare results from multiple trials. However, creating good tests is time-consuming. Look for a test in the near future as I update the PR.
There was a problem hiding this comment.
sure, if you can save the test data for 2 trials with the default parameters and send it to me, I'll add that as a test. For now, I merged since it does not break anything for single trial.
|
I'm merging this to move on. |
| net.spiketimes = h.Vector() | ||
| net.spikegids = h.Vector() | ||
| net._record_spikes() | ||
|
|
There was a problem hiding this comment.
Edit: sorry, should have made a new issue. See #78
@jasmainak I'm preparing a PR to add ParallelContext (n_cores > 1, but not compatible with n_jobs > 1). A function similar to _clone_and_simulate will be called, and the net.* functions need to be part of it. Can you explain why these functions need to separate from Network(params, n_cores)? All of _clone_and_simulate gets pickled, right? Perhaps these net.* calls can be part of an appropriately named method of Network.
@blakecaldwell thanks to your PR I think we might be able to get embarrassingly parallel working pretty easily! This is a bit WIP but I'm quite positive it will work. More soon ...