-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENH: Add somato example #35
Conversation
Codecov Report
@@ Coverage Diff @@
## master #35 +/- ##
=======================================
Coverage 76.17% 76.17%
=======================================
Files 11 11
Lines 1356 1356
=======================================
Hits 1033 1033
Misses 323 323 Continue to review full report at Codecov.
|
1d60523
to
3ffedbd
Compare
@rythorpe @blakecaldwell do you want to try this example out and review the code? I updated it with the parameters that I got from Ryan. I have two questions:
Let me know. |
examples/plot_simulate_somato.py
Outdated
params_fname = op.join(mne_neuron_root, 'param', 'default.json') | ||
params = Params(params_fname) | ||
|
||
params.update({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rythorpe I would suggest sharing the params as a "diff". That is, what do we need to change in the defaults.param or defaults.json to get the waveform. That way, it's a bit more clear to the users (and to me!) what are the canonical parameters that can be somewhat fixed and which ones are actually critical to producing the waveform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The waveform above is a bit different from the one produced by HNN. Here's what I get:
We could do that, though we would need to come to some sort of consensus regarding what we consider to be the "default" parameters. Were you thinking of just using the default.param file referenced in the HNN tutorial?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm ... the difference is probably because you have multiple trials. I made n_trials = 1 because we're still trying to figure out what's the best way to do the parallel processing with @blakecaldwell. There is an open pull request for that. There seems to be a fair amount of noise at the level of single trials. Did you try and see what it looks like for n_trials = 1? It should be the same because the random seeds etc have not changed.
Yes, currently I am just using the default.param from the HNN tutorials. But we could change this of course. Just want to change one thing at a time so we know where the differences are coming from (between softwares, models, examples etc.), if any.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasmainak What are the units on the y-axis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be the same as yours, that is nAm with a scaling factor of 30. Where do you get the 6000 cells from? It's not in your param file ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 6000 (pyramidal) cells comes from the number of pyramidal cells per layer (Local Network Parameters->Cells->[Num Pyr Cells] for the x and y-dimensions) times the number of layers (2, corresponding to L2/3 and L5 Pyr) times the scaling factor. For a scaling factor of 30, this is 10x10x2x30=6,000. For the default.param simulation, it is 10x10x2x3000=600,000 and reflects a greater number of aligned pyramidal cells contributing to the net current dipole.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I see, thanks for explaining. The plots look very similar to me -- the only difference being that you have smoothing over many trials. I think the next logical step would be to get back to #40 and see what can be done to make it work in parallel for multiple trials.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rythorpe I am merging this for now to move forward. I will raise an issue to dig into this. It will require more work. I need to think of a good way to debug. Thanks for helping! |
cc @rythorpe
this gives the following figure:
we can probably do a better job, but it's already a start.