-
Notifications
You must be signed in to change notification settings - Fork 163
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] Drop _part-
reference from example, introduce _split-
entity
#460
Conversation
FieldTrip does deal with split files in general, however not specifically for FIF but using a general mechanism. Please note that not everyone contributing to the mailing list might be aware of this. See https://github.com/fieldtrip/fieldtrip/blob/d8662e204096419b2bfafcc64910e12665368209/fileio/ft_read_header.m#L124, you would use this by specifying
FieldTrip does (up to now) not autodetect neuromag fif files that should go together, in that sense it behaves similar as most of the vendor software (Neuromag/Elekta/MEGIN) that also does not do that. So it is the responsibility of the user. Regarding the comparison to BrainVision: yes, I see there is a resemblance of files pointing to each other (in the Neuromag case as a linked-list) where the file names are explicitly coded inside the header content. In most cases (multi-file formats) that I am aware of, there are no explicit pointers between files, but only implicit ones, where links between files are implemented based on the filenames alone. Putting filenames inside the binary content of files IMHO messes up the relation between metadata (the name) and the data (the content) and is prone to problems. E.g renaming requires special tools, symbolic linking does not work, there is the risk of the path (e.g with |
Having said this, I do agree with the proposed pull-request: I do not see it as conflicting with the current interpretation, and it makes it more clear. |
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.
LGTM so far!
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.
There is no <id>
, see further points in the comments
src/99-appendices/04-entity-table.md
Outdated
| events<br>(meg/eeg/ieeg) | REQUIRED | OPTIONAL | REQUIRED | | | | | OPTIONAL | | | | | | | ||
| Entity | Subject | Session | Task | Acquisition | Contrast Enhancing Agent | Reconstruction | Phase-Encoding Direction | Run | Corresponding modality | Echo | Recording | Processed (on device) | Space | Part | | ||
| :--------------------------------------------------------------------------------------------- | :------------ | :------------ | :------------- | :------------ | :----------------------- | :------------- | :----------------------- | :------------ | :--------------------- | :------------- | :------------------ | :-------------------- | :---------------| :------------- | | ||
| Format | `sub-<label>` | `ses-<label>` | `task-<label>` | `acq-<label>` | `ce-<label>` | `rec-<label>` | `dir-<label>` | `run-<index>` | `mod-<label>` | `echo-<index>` | `recording-<label>` | `proc-<label>` | `space-<label>` | `part-<index>` | |
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.
see other comment about <index>
vs <label>
- let's see if it could be relaxed here to be <label>
right away.
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 had not interpreted <index>
as index into something explicitly (as in expressing a link), rather as how you would use it in a for-loop, which means a sequence of integers that increment by one. The starting value is not defined AFAIK, and I would expect it either to be 0 or 1.
The run entity is the earliest use of <index>
, but it is also used (in the same part of the spec) for the echo entity (although not explicitly defined there, it is in examples and in the entity appendix table).
The use of part-
in MEG requires it to be <index>
and that is also how it is specified now, so I think the fix in this PR is correct (as long as <id>
is changed to <index>
). Extending the value to <label>
would be another discussion. Right now I don't think we have consensus yet on re-using part (with a different meaning of the entity) for non-MEG data, so I think it would be too early to decide for loosening it up from index to label.
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.
Re <index>
as an explicit index, it might be coming as a part of BEP001 (ideally should be a separate BEP or a PR first): https://github.com/bids-standard/bep001/blob/indexable_metadata/src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md#the-indexable_metadata-entity . ATM we use it only for run and echo. Indeed the value of <index>
is not yet clearly defined/controlled in BIDS otherwise.
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 use of part- in MEG requires it to be ...
That was my question really in above comment -- is it required? For clarity: is the only tool in question (mne-python) relies on that index, or users would just point to the first file in the sequence and then internal pointer to the next file is used? I saw use of _construct_bids_filename
(the only place I found _part-
mentioned) only within write_data
but have not spotted it elsewhere (somewhere in reading).
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.
Internal pointing can IMHO not be relied on since for most users it will not be possible (i.e. they do not have the tools) to update these internal pointers when they rename the file. AFAIK the only tool that can be used for updating these internal pointers is MNE-Python. I do not recall ever seeing such a tool from the vendor itself.
Putting the responsibility with the user to provide the files in the right sequence (as we e.g. do in FIeldTrip) would always be possible.
|
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
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.
LGTM
_part-
reference from example, introduce _split-
entity
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.
Looks good. Some minor pedantry.
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
Merging now as we have a sufficient amount of community and maintainer approval. Robert has approved elsewhere (mne-tools/mne-python#7776 (comment)) |
* origin/master: (404 commits) [DOC] Auto-generate changelog entry for PR bids-standard#460 Apply suggestions from code review label -> index drop _part-, introduce _split- [DOC] Auto-generate changelog entry for PR bids-standard#459 [DOC] Auto-generate changelog entry for PR bids-standard#465 fix table Update src/99-appendices/06-meg-file-formats.md [DOC] Auto-generate changelog entry for PR bids-standard#441 inject _part into MEG spec update entity table FIX: clarify _part Apply suggestions from code review FIX: clarify participants tsv [DOC] Auto-generate changelog entry for PR bids-standard#457 Update Release_Protocol.md add pdf steps for release protocol FIX: remove BESA from list of restricted keywords Remove trailing space Add reference to PDF on front page of specification ... Conflicts: src/02-common-principles.md - had to meld with my previous wording etc.
* upstream/master: (113 commits) [DOC] Auto-generate changelog entry for PR bids-standard#152 [DOC] Auto-generate changelog entry for PR bids-standard#467 Specify that suffix must be alphanumeric ENH: make NOT RECOMMENDED stronger (SHOULD NOT) for zero padding for uniqueness ENH: Include leading . within definition of the file extension ENH: provide an example for a suffix based on an _eeg.vhdr filename [DOC] Auto-generate changelog entry for PR bids-standard#477 [DOC] Auto-generate changelog entry for PR bids-standard#460 Also ignore users urls on github Quote regexp in command line [INFRA] linkchecker - ignore github pull and tree URLs Apply suggestions from code review replace purview with scope label -> index Apply suggestions from code review drop _part-, introduce _split- Apply SA feedback and amended to purview [DOC] Auto-generate changelog entry for PR bids-standard#459 Add Domain Expert to Maintainers Group [DOC] Auto-generate changelog entry for PR bids-standard#465 ...
closes #429
This PR is intended to result in a solution for #429. The proposed changes here should make it easier to discuss how to resolve the issue.
currently proposed changes aim at keeping the_part
entity in BIDS and documenting it in a proper way. The_part
entity is defined to indicate parts of a split file.Given the developments in mne-tools/mne-python#7776 I changed the title and proposed changes of this PR -> Drop
_part-
reference from example, introduce_split-
entityTo do
part
-->split
?_part
find . -name '*_part*'
Reading this fieldtrip mail it seems to me, that FieldTrip does not deal with split FIF files at all --> @robertoostenveld to me this seems like a similar problem as with BrainVision, where the
vhdr
file contains pointers to the accompanyingeeg
andvmrk
files.When renaming the files, their counterparts SHOULD (I think even MUST) be renamed --> yet for FIF, how did you deal with this so far?