Skip to content
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

[MRG] DOC: Update alpha example #249

Merged
merged 17 commits into from
Jan 27, 2021
Merged

Conversation

ntolley
Copy link
Contributor

@ntolley ntolley commented Jan 20, 2021

Adding more detailed descriptions to alpha wave example. Additionally updating simulation to match alpha and beta tutorial provided in HNN-GUI (https://jonescompneurolab.github.io/hnn-tutorials/alpha_and_beta/alpha_and_beta)

@jasmainak
Copy link
Collaborator

Let me know when it's ready!

@jasmainak
Copy link
Collaborator

jasmainak commented Jan 25, 2021

you need to rebase

also the CircleCI error can be resolved in many ways

  1. Increasing the CircleCI timeout
  2. Changing the sphinx building to be more verbose (currently it suppresses example outputs)
  3. Separating the example into two

Choose your poison but I'd prefer 2. or 3.

@cjayb
Copy link
Collaborator

cjayb commented Jan 25, 2021

What's the reason for increasing the simulation time from 310 to 710? I get it from a physiological perspective, but to keep the examples running in reasonable time, I'd prefer we stuck to the max 300 we have in all other cases.

Doing low-F TFRs gets tricky, so we should consider not including too many of them. The spectrum isn't as pretty, but how about just using that in this example?

Copy link
Collaborator

@cjayb cjayb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in the comments, really think this should be a 300 ms simulation. Even at the cost of a pretty TFR...

examples/plot_simulate_alpha.py Outdated Show resolved Hide resolved
examples/plot_simulate_alpha.py Outdated Show resolved Hide resolved
examples/plot_simulate_alpha.py Outdated Show resolved Hide resolved
examples/plot_simulate_alpha.py Outdated Show resolved Hide resolved
examples/plot_simulate_alpha.py Outdated Show resolved Hide resolved
Comment on lines +58 to +47
syn_delays_p = {'L2_basket': 0.1, 'L2_pyramidal': 0.1,
'L5_basket': 1., 'L5_pyramidal': 1.}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
syn_delays_p = {'L2_basket': 0.1, 'L2_pyramidal': 0.1,
'L5_basket': 1., 'L5_pyramidal': 1.}
syn_delays_p = {'L2_pyramidal': 0.1, 'L5_pyramidal': 1.}

Comment on lines +88 to +80
syn_delays_d = {'L2_basket': 5., 'L2_pyramidal': 5.,
'L5_basket': 5., 'L5_pyramidal': 5.}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
syn_delays_d = {'L2_basket': 5., 'L2_pyramidal': 5.,
'L5_basket': 5., 'L5_pyramidal': 5.}
syn_delays_d = {'L2_pyramidal': 5., 'L5_pyramidal': 5.}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these default values for basket cells or something? In any case I am getting this error when I try it:
ValueError: synaptic_delays is either a common float or needs to be specified as a dict for each cell type

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope: the delays need to be specified for any cell type for which weights are given. Since you're only connecting to pyramidal cells, there's no reason to include basket delays in the dict.

This works for me on this branch, could you try again before we investigate further?

examples/plot_simulate_alpha.py Outdated Show resolved Hide resolved
@ntolley
Copy link
Contributor Author

ntolley commented Jan 25, 2021

I'll see how everything looks with the 300 ms simulation. The main motivation to increase the timing was to match the alpha/beta tutorial for HNN-GUI. It is necessary because the only beta event in the simulation occurs after 300 ms: https://jonescompneurolab.github.io/hnn-tutorials/alpha_and_beta/alpha_and_beta.

Since the timing of beta events is stochastic, I would need to find a seed that produces a beta event within 0-300 ms. I'm not against this, but it's just another question of how much congruency we want to maintain with the GUI tutorials.

@ntolley
Copy link
Contributor Author

ntolley commented Jan 25, 2021

Choose your poison but I'd prefer 2. or 3.

@jasmainak I think the path will just be decreasing the simulation time and changing the seeding to produce beta events earlier in the simulation. I started making a separate example for just beta events but it seemed pretty redundant as I got working. The only distinction from the alpha example is a second 10 Hz rhythmic drive.

@cjayb
Copy link
Collaborator

cjayb commented Jan 25, 2021

I think the path will just be decreasing the simulation time and changing the seeding to produce beta events earlier in the simulation

I agree! It should be possible to /infer/ the seed need to produce the ~450 ms event, but it's probably easier to go with trial-and-error ;)

params_fname = op.join(hnn_core_root, 'param', 'default.json')
params = read_params(params_fname)
print(params)

###############################################################################
# Update a few of the default parameters related to visualisation
# Next we update a few of the default parameters related to visualisation. The
# ``dipole_scalefctr`` relates to the amount of cortical tissue necessary to
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should instead add this line at the end:

simulate_dipole(..., postproc=False)
dpl.scale(15000)

to discourage fiddling with params as much as possible.

This also makes me realize that Dipole is not documented in the API reference. Do you mind adding it there?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1!

I’d go as far as to stop reading any post-proc details from params (got bit in the rear by a dipole_smooth_win setting of 30, set to zero above). future PR...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion! I've removed all params edits except for tstop now,

@jasmainak
Copy link
Collaborator

jasmainak commented Jan 25, 2021

@ntolley thanks for addressing all our whims!

so the beta rhythm doesn't really look rhythmic to me and the previous alpha rhythm was more rhythmic. Does it look rhythmic when you plot until 710 ms? Spectrogram plots can be deceptive as even a small non-stationarity in the signal can show up as a rhythm. I think I prefer TFR for this reason as you can see clearly then how long is the beta -- whether it's something long enough to be considered a brain rhythm.

Many people also use Welch instead of regular PSD ... and you'll see this peak disappear if you use Welch.

Not to question everything but perhaps something to keep in mind? Thoughts?

@ntolley ntolley changed the title DOC: Update alpha example [MRG] DOC: Update alpha example Jan 25, 2021
@ntolley
Copy link
Contributor Author

ntolley commented Jan 25, 2021

@jasmainak it's a valid point, the beta rhythm isn't really a rhythm which is highly emphasized in the references/GUI tutorial. At the same time I don't think this negates the utility of power spectra for identifying discrete beta events (in fact they are present in the GUI tutorial). If we really want to get into the nuance of the phenomena and how it's measured this example would need to be a bit longer.

Also it's not exactly clear in the TFR that these beta events are in fact discrete events when using shorter simulations. In light of at least showing beta band activity they serve the same purpose (although I'll need to check on the welch analysis...)

If you feel strongly for the TFR plots I can change over the plots.

@codecov-io
Copy link

codecov-io commented Jan 26, 2021

Codecov Report

Merging #249 (7fd2944) into master (13c2dfe) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #249   +/-   ##
=======================================
  Coverage   52.93%   52.93%           
=======================================
  Files          23       23           
  Lines        2747     2747           
=======================================
  Hits         1454     1454           
  Misses       1293     1293           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13c2dfe...7fd2944. Read the comment docs.

@jasmainak
Copy link
Collaborator

okay all good. Last question -- are we okay to have the alpha rhythm deviate from the example on the HNN website?

@ntolley
Copy link
Contributor Author

ntolley commented Jan 26, 2021

@rythorpe what do you think? My feeling is it's ok to deviate if we're already truncating the recording to half the original length. But it's a pretty simple fix to change the seeding back. I'll just have to play with the seeding of the second inputs that are added to ensure a beta event occurs within the time window.

@jasmainak
Copy link
Collaborator

well this example was kind of a visual check for the rhythmic drive since we don't have a proper test for it. My eye had gotten used to spotting any discrepancies. But I won't insist on keeping the old waveforms if you guys are okay with changing. I let @rythorpe merge if he's happy!

Copy link
Collaborator

@cjayb cjayb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many people also use Welch instead of regular PSD ... and you'll see this peak disappear if you use Welch.

Good point. The current implementation uses spectrogram with n_fft set to the entire window. I think a more principled approach to both spectra and tfr's are called for. I think we should optimise the examples for aesthetics.

examples/plot_simulate_alpha.py Outdated Show resolved Hide resolved
examples/plot_simulate_alpha.py Outdated Show resolved Hide resolved
examples/plot_simulate_alpha.py Show resolved Hide resolved
@cjayb
Copy link
Collaborator

cjayb commented Jan 26, 2021

Looking good! I tried to fiddle with different TFR settings, but it's hard to make anything illustrative with only one trial (which is very much to the point!). The spectra do the trick of underlining the point.

Take a look at Figure 4 in Sherman et al. (2016) below, which hnn_core can clearly reproduce:

Screenshot 2021-01-26 at 15 03 10

The beta-events appear because the proximal drive has a wider burst_std compared with the distal one.

@ntolley to 'guarantee' beta event, you could reduce burst_std to 15 or even 10 ms for the second distal drive. I would also modify the comment/explanation in the code to something like:

# The next step is to add a simultaneous 10 Hz distal drive with a lower
# within-burst spread of spike times (``burst_std``) compared with the
# proximal one. The different arrival times of spikes at opposite ends of
# the pyramidal cells will tend to produce bursts of 15-30 Hz power known
# as beta frequency events.

ntolley and others added 11 commits January 26, 2021 13:39
Co-authored-by: Christopher J. Bailey <bailey.cj@gmail.com>
Co-authored-by: Christopher J. Bailey <bailey.cj@gmail.com>
Co-authored-by: Christopher J. Bailey <bailey.cj@gmail.com>
Co-authored-by: Christopher J. Bailey <bailey.cj@gmail.com>
Co-authored-by: Christopher J. Bailey <bailey.cj@gmail.com>
@jasmainak
Copy link
Collaborator

merge button is yours @cjayb ! All good on my end

@cjayb cjayb merged commit acef0cd into jonescompneurolab:master Jan 27, 2021
cjayb added a commit to cjayb/hnn-core that referenced this pull request Jan 27, 2021
@cjayb
Copy link
Collaborator

cjayb commented Jan 27, 2021

Hurra! See follow-up at #264

cjayb added a commit to cjayb/hnn-core that referenced this pull request Feb 2, 2021
jasmainak pushed a commit that referenced this pull request Feb 25, 2021
@ntolley ntolley mentioned this pull request Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants