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

[FIX] Remove father-level for meg filetypes other than BTi/4D data #19

Merged
merged 3 commits into from
Mar 28, 2019

Conversation

teonbrooks
Copy link
Collaborator

before the document migration, we were wrapping up discussion on removing the father-level of folders for all MEG formats except for BTi/4D. The premise of the father-level directory was to compensate for the fact that BTi data is written without file extension. Its data files are generated with the assumption that they all belong to the same folder.

we proposed adding an additional folder level for this data format. it was extended to a few other file formats but we have realized that this is unnecessary breaks the general pattern. we then proposed to remove the father-level for the file types affected by the superfluous folder level with the exception of BTi where the folder isn't superfluous but necessary.

@chrisgorgo
Copy link
Contributor

@chrisgorgo chrisgorgo added the MEG Magnetoencephalography label Oct 7, 2018
@teonbrooks
Copy link
Collaborator Author

here's the discussion I had with @robertoostenveld from the google doc.

screenshot_2018-11-03 the brain imaging - this is the level that i think matt i - teon brooks gmail com - gmail

src/99-appendices/06-meg-file-formats.md Outdated Show resolved Hide resolved
src/99-appendices/06-meg-file-formats.md Outdated Show resolved Hide resolved
sappelhoff and others added 2 commits March 23, 2019 13:13
Co-Authored-By: teonbrooks <teon.brooks@gmail.com>
Co-Authored-By: teonbrooks <teon.brooks@gmail.com>
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.

@robertoostenveld @monkeyman192 could you find some time to review this?

It looks good to me but I'd like some MEG expert input

Copy link
Collaborator

@robertoostenveld robertoostenveld left a comment

Choose a reason for hiding this comment

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

I think it would be good to make the initial table more consistent, i.e. in the definition (2nd column) only list the timeseries data (plus header and/or events), or to list the extension of all files that are mentioned in the manufacturer specific paragraph. Either one is fine with me.

for KIT .mrk is listed in the table. I think it is for head markers, not events. But I am not sure. If all files are in the table, then also the others should be completed in the table.

For CTF the .shape and .shape_info are completely missing, they are supported by the validator.

For KRISS the .chn and .trg are listed in the paragraph, not in the table.

Aalto MEG should refer to "Neuromag/Elekta/MEGIN" format (now MEGIN is not part of it).

But if you want to keep the PR small, I am fine and approve these changes.

@monkeyman192
Copy link
Collaborator

I think having all the raw files in the same folder is a good idea (I think I brought it up initially in the google docs.)
The changes look good and I think that the changes @robertoostenveld mentioned are probably better done in a separate PR to keep the issues separate.
I just have the one suggested change but LGTM otherwise :)

rename/create a father run specific directory and keep the original files for
each run inside.
Each experimental run on a KIT/Yokogawa/Ricoh system yields a raw
(`.sqd`, `.con`) file with its associated marker coil file (`.mrk`), which
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should add that .sqd is a valid marker file extension also

@sappelhoff
Copy link
Member

Thanks for the review @monkeyman192 I am merging this now and will ask you to incorporate your review comment in your own PR #62 because you are editing the same file there and both PRs are connected (slightly) :-)

@teonbrooks will probably be happy to have this closed after 5 months 😆

Thanks everybody

@sappelhoff sappelhoff changed the title remove father-level for meg filetypes other than BTi/4D data [FIX] Remove father-level for meg filetypes other than BTi/4D data Mar 28, 2019
@sappelhoff sappelhoff merged commit e28d648 into bids-standard:master Mar 28, 2019
@jasmainak
Copy link
Collaborator

Does this need any updates in the validator?

@sappelhoff
Copy link
Member

Does this need any updates in the validator?

see mne-tools/mne-bids#187 (comment)

@teonbrooks
Copy link
Collaborator Author

awesome! @sappelhoff, you're correct, i am pleased :)

@teonbrooks teonbrooks deleted the meg-father-level-removal branch March 31, 2019 19:25
monkeyman192 added a commit to monkeyman192/bids-specification that referenced this pull request Apr 5, 2019
sappelhoff pushed a commit that referenced this pull request Apr 23, 2019
sappelhoff pushed a commit that referenced this pull request Jul 25, 2019
sappelhoff added a commit that referenced this pull request Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MEG Magnetoencephalography
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants