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] Read_raw extension to extend sidecar json func. #341

Merged
merged 31 commits into from
Jan 23, 2020

Conversation

adam2392
Copy link
Member

@adam2392 adam2392 commented Jan 3, 2020

PR Description

Extends read_raw functionality to extract data from the sidecar json file. Closes: #340

Also includes check on data elements, such as:

  • Reference
  • SamplingFrequency

Merge checklist

Maintainer, please confirm the following before merging:

  • All comments resolved
  • This is not your own PR
  • All CIs are happy
  • PR title starts with [MRG]
  • whats_new.rst is updated
  • PR description includes phrase "closes <#issue-number>"
  • Commit history does not contain any merge commits

@jasmainak
Copy link
Member

any test to update?

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

Thanks for the contributions. I made some comments.

Also:

  • What happens if we CAN read a line_freq from the JSON, and the raw.info[line_freq] is None? --> In that case it'd probably be good to just set the raw.info[line_freq] according to what we read, instead of raising an error

mne_bids/read.py Outdated Show resolved Hide resolved
mne_bids/read.py Outdated Show resolved Hide resolved
mne_bids/read.py Outdated Show resolved Hide resolved
mne_bids/read.py Outdated Show resolved Hide resolved
@adam2392
Copy link
Member Author

adam2392 commented Jan 6, 2020

any test to update?

I checked test_read.py, but there were not specific unit tests for read_raw_bids. Do you want me to write an additional test? If so, what should it look like?

@jasmainak
Copy link
Member

see how we do the rest of the tests. Check if the errors are raised etc.

also you need to rebase to make the CIs happy. Good luck! :)

@codecov-io
Copy link

codecov-io commented Jan 6, 2020

Codecov Report

Merging #341 into master will increase coverage by 0.23%.
The diff coverage is 98.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #341      +/-   ##
==========================================
+ Coverage    94.9%   95.14%   +0.23%     
==========================================
  Files          11       11              
  Lines        1218     1277      +59     
==========================================
+ Hits         1156     1215      +59     
  Misses         62       62
Impacted Files Coverage Δ
mne_bids/write.py 97.27% <100%> (+0.2%) ⬆️
mne_bids/read.py 99.41% <100%> (+0.07%) ⬆️
mne_bids/utils.py 98.3% <97.43%> (-0.14%) ⬇️

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 a54d616...d82ccb7. Read the comment docs.

@jasmainak
Copy link
Member

@adam2392 can you see the checklist above? You need to say "closes #340" or "fixes #340". I don't think "addresses" will do it.

Also codecov is failing because you didn't add a test.

mne_bids/read.py Outdated Show resolved Hide resolved
mne_bids/read.py Outdated Show resolved Hide resolved
@jasmainak jasmainak added this to the 0.4 milestone Jan 8, 2020
@jasmainak
Copy link
Member

seems like you need to rebase

@adam2392 adam2392 changed the title Read_raw extension to extend sidecar json func. [MRG] Read_raw extension to extend sidecar json func. Jan 8, 2020
@adam2392 adam2392 changed the title [MRG] Read_raw extension to extend sidecar json func. Read_raw extension to extend sidecar json func. Jan 8, 2020
@adam2392 adam2392 changed the title Read_raw extension to extend sidecar json func. [MRG] Read_raw extension to extend sidecar json func. Jan 8, 2020
@adam2392 adam2392 changed the title [MRG] Read_raw extension to extend sidecar json func. [WIP] Read_raw extension to extend sidecar json func. Jan 8, 2020
@jasmainak
Copy link
Member

I had a brief look at the automatic power line detection. That’s cool! Couple of comments:

  • can we avoid making a big copy of the raw data? Computing PSD on one data channel is enough?

  • is the peak detection robust? If not, you could just compare the power at 50 and 60 Hz.

Sorry I can’t look in more detail now ...

@adam2392
Copy link
Member Author

adam2392 commented Jan 9, 2020

I had a brief look at the automatic power line detection. That’s cool! Couple of comments:

  • can we avoid making a big copy of the raw data? Computing PSD on one data channel is enough?
  • is the peak detection robust? If not, you could just compare the power at 50 and 60 Hz.

I make a big copy, so that we can make use of all channels with sampling data. Assuming that there isn't severely corrupted data, then this should work robustly due to the fact that averaging many channels (with possible non-noise power at 50/60 Hz ranges) gets rid of the signal, and amplifies common noise.

I agree that probably "most" of the time, a single channel suffices, but I figured this could potentially cover essentially "all" cases, since it is a more robust estimate of what frequency is common across many electrodes (e.g. line noise). I trimmed the algorithm to only analyze the minimum of 10 seconds, or the whole dataset. I'm not sure if MNE-Python has the capability of slicing the dataset before loading?

Sorry I can’t look in more detail now ...

Enjoy your vacation :). I'm just putting these in the pipeline to get them out of my way before I forget what I wanted to do.

mne_bids/utils.py Outdated Show resolved Hide resolved
@adam2392
Copy link
Member Author

    picks = pick_types(raw.info, meg=True, eeg=True,
                       stim=True, ecog=True, seeg=True,
                       exclude='bads')
    ch_names = [raw.ch_names[k] for k in picks]

    # first bandpass the signal to just between 45 and 65 Hz
    sfreq = raw.info['sfreq']
    l_freq = 45
    h_freq = min(sfreq // 2 - 1, 65)

    # only sample first 10 seconds, or whole dataset
    tmin = 0
    tmax = max(len(raw.times), 10 * sfreq)
    data = raw.get_data(start=tmin, stop=tmax, picks=picks, return_times=False)
    data = filter_data(data, sfreq=sfreq,
                       l_freq=l_freq, h_freq=h_freq)

    # run a multi-taper FFT
    psds, freqs = psd_array_multitaper(data, sfreq=sfreq,
                                       fmin=45, fmax=65,
                                       n_jobs=1)
    
    # find the maximum occurrence of frequency and if it is close to line
    psds = np.mean(np.abs(10 * np.log10(psds)), axis=0)
    if len(freqs) != len(psds):
        raise RuntimeError("{} != {}".format(len(freqs), len(psds)))
    possible_power_lines = [50, 60]
    max_freq = freqs[np.argmax(psds)]
    estimated_index = np.argmax([abs(max_freq - x)
                                 for x in possible_power_lines])
    powerlinefrequency = possible_power_lines[estimated_index]
    return powerlinefrequency

Could be used I suppose, but yeah you're right, probably better to just check frequencies around 50 and 60 Hz and compare average PSD for all channels.

mne_bids/utils.py Outdated Show resolved Hide resolved
mne_bids/utils.py Outdated Show resolved Hide resolved
mne_bids/utils.py Outdated Show resolved Hide resolved
mne_bids/utils.py Outdated Show resolved Hide resolved
@jasmainak jasmainak merged commit 4d8c279 into mne-tools:master Jan 23, 2020
@jasmainak
Copy link
Member

Thanks @adam2392 !

@jasmainak
Copy link
Member

Let's close the other two PRs as well ;-)

@adam2392 adam2392 deleted the read_raw branch February 4, 2020 00:50
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.

read_raw_bids should read in sidecar json file to set line frequency
5 participants