-
Notifications
You must be signed in to change notification settings - Fork 169
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
[FIX] Remove father-level for meg filetypes other than BTi/4D data #19
Conversation
This has been brought up by @teonbrooks before (https://groups.google.com/d/msgid/bids-discussion/CAGdVzab%3DSufiv4Qs4CWwSLTDXs0C6sfMVJG3YH4Vz0VT%2BdyaOw%40mail.gmail.com?utm_medium=email&utm_source=footer). It would be great to get @guiomar and @robertoostenveld opinion. |
bfa5e0a
to
58b038f
Compare
58b038f
to
c05b3ea
Compare
here's the discussion I had with @robertoostenveld from the google doc. |
Co-Authored-By: teonbrooks <teon.brooks@gmail.com>
Co-Authored-By: teonbrooks <teon.brooks@gmail.com>
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.
@robertoostenveld @monkeyman192 could you find some time to review this?
It looks good to me but I'd like some MEG expert input
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 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.
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.) |
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 |
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.
You should add that .sqd
is a valid marker file extension also
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 |
Does this need any updates in the validator? |
|
awesome! @sappelhoff, you're correct, i am pleased :) |
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.