-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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] Conversion of somato data to BIDS #6414
Conversation
It seems like the BIDS naming clashes with the MNE naming convention: This reminds me of: #4950 |
yes this has never been fixed in mne-python but it should not warn for
raw bids files
|
I'll give it a shot |
can you share link to rendered examples? |
sure, but they did not run completely due to the runtime warning that the filename for RAW does not follow MNE conventions:
|
ah okay, let us know when that works. Also I think I prefer to change mne_source_alignment to mne_coregistration |
I would try to be as much close to BEP003 as possible. So we need to do minimal updates later. For example, you need dataset_description.json:
Similarly:
|
I agree, the most current BEP for derivatives is here: bids-standard/bids-specification#207 |
Would these be the correct places to change the naming convention (i.e., accept BIDS endings that are Lines 1572 to 1574 in e1a160c
and Lines 136 to 138 in e1a160c
It would also require changing a couple of docstrings and also some docs, such as |
yep! Don't break the report while you do so. I think it expects certain file format in report.parse_folders() :-) |
Codecov Report
@@ Coverage Diff @@
## master #6414 +/- ##
==========================================
- Coverage 89.4% 89.39% -0.01%
==========================================
Files 415 415
Lines 74985 74985
Branches 12330 12330
==========================================
- Hits 67039 67035 -4
- Misses 5118 5120 +2
- Partials 2828 2830 +2 |
@larsoner the new somato data is not found for building the docs ... could this be because we are caching it and the cache is still old? I changed the md5 hash and the URL though: https://github.com/mne-tools/mne-python/pull/6414/files#diff-00583e36a75a694e071f9fc054a71c08 |
The error is:
When I download your
|
🤦♂️ okay, I re-uploaded with proper folder naming. Let's see how this goes. PS: Thanks a lot for spotting it |
@sappelhoff did you run the dataset through the validator? any issues you faced? Is it compliant? otherwise PR looks clean to me. |
yep, see: (base) stefanappelhoff@arc-lin-004309:~/Desktop$ bids-validator MNE-somato-data
This dataset appears to be BIDS compatible.
Summary: Available Tasks: Available Modalities:
11 Files, 334.48MB somato T1w
1 - Subject coordsystem
1 - Session channels
events
meg
If you have any questions, please post on https://neurostars.org/tags/bids. I also added |
sounds good. +1 to merge from my end once CIs are happy |
just curious @sappelhoff, are you adding these derivative files to the BEP 021? |
examples/time_frequency/plot_time_frequency_global_field_power.py
Outdated
Show resolved
Hide resolved
@teonbrooks not yet - but we just had a BIDS discussion at OHBM and we'll get more focused on the derivatives starting soon :-) ... probably with a survey of what most people need (doing a "data driven 80-20" as @jasmainak put it ;-) ) |
@agramfort with using anchors: do you mean I should do it like this? (see below) .. _`somatosensory dataset`: https://martinos.org/mne/stable/manual/datasets_index.html?#somatosensory
=========================================
Compute source power using DICS beamfomer
=========================================
Compute a Dynamic Imaging of Coherent Sources (DICS) [1]_ filter from
single-trial activity to estimate source power across a frequency band. This
example demonstrates how to source localize the event-related synchronization
(ERS) of beta band activity in the `somatosensory dataset`_ |
you need to insert
.. _somato-dataset:
in L124 in datasets_index.rst
and then you'll be able to refer to this section with :ref:`somato-dataset`
|
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.
I'm blocking the PR so as to remind us that we should use the digitization coordinates as in the spec instead of the trans files.
to remind myself, this is the rst docs section dealing with that: https://www.sphinx-doc.org/en/1.5/markup/inline.html#cross-referencing-arbitrary-locations |
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.
besides subj -> subject LGTM
Thanks @sappelhoff ! |
subject = '01' | ||
task = 'somato' | ||
raw_fname = op.join(data_path, 'sub-{}'.format(subject), 'meg', | ||
'sub-{}_task-{}_meg.fif'.format(subject, task)) |
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.
I know I'm late on the review cycle and this is merged, but do you guys find worth to create a helper function that allows composing the name?
def fname_helper(subject, task='somato', data_type_prefix=''):
return op.join(
'sub-{0:02d}'.format(subject),
data_type_prefix,
'sub-{0:02d}_task-{}_meg.fif'.format(subject, task)
)
from mne.datasets.somato import data_path, fname_helper
raw_fname = op.join(data_path(), fname_helper(subject=1, data_type_prefix='meg'))
fname_fwd = op.join(data_path, 'derivatives', fname_helper(subject=1))
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.
ups !! the file names are not the same.
one is _meg.fif
the other one is _fwd.fif
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 derivatives is not set in stone, so I would not make a new function. There is already one in mne-bids for the existing BIDS spec: https://mne.tools/mne-bids/generated/mne_bids.make_bids_basename.html#mne_bids.make_bids_basename.
Also reading the artifacts I found that we are using a private function in a tutorial:
Shall we make the function public? wrap wherever is exposed and make this one public? |
Also reading the artifacts I found that we are using a private function in
a tutorial:
https://github.com/mne-tools/mne-python/blob/cc836c2635b7a720ec6c60074a1997fe52d7ca2f/examples/time_frequency/plot_time_frequency_global_field_power.py#L56
Shall we make the function public? wrap wherever is exposed and make this
one public?
nice catch ! this is evil. @sappelhoff can you fix? or anyone?
|
Can @massich elaborate a bit on his intention for the fix? |
you're not supposed to use private functions in examples ... |
Yes, I learned that repeatedly through you and Alex :-D But I was wondering what @massich meant with
|
well if you wrap another function around it, you can potentially expose fewer parameters publicly while still keeping the flexibility of the private function. Not sure if that's necessary though. |
* adjust freq tut * adjust dics exp * adjust global field power exp * temporarily change hash and url for somato * make more use of subj variable * use v2 of somato bids set * ignore long lines due to links in rst * fix typo * add somato url from MNE osf repo * fix sphinx rst links * update examples for new formatting, no new data yet * update to most recent somato data * update * fix path * fix rst link * subj --> subject
@sappelhoff looks like the # in ~/mne_data/MNE-somato-data/derivatives/freesurfer/subjects
model = mne.make_bem_model('01', conductivity=(0.3,), subjects_dir='.', verbose=True)
mne.write_bem_surfaces('01/bem/01-5120-bem.fif', model)
bem = mne.make_bem_solution(model, verbose=True)
mne.write_bem_solution('01/bem/01-5120-bem-sol.fif', bem) |
Can confirm -- tried to run some older code after updating |
sounds like an oversight to me because I have no clue what these files do. This is the conversion script: https://github.com/mne-tools/mne-scripts/blob/master/bids_conversion/convert_somato_data.py As long as the files are being put into the |
Okay, will push a commit there and update the dataset on OSF |
thanks a lot @larsoner and sorry to cause problems. |
for reference, see this gitter chat
EDIT: check out the conversion script
Current structure: