Extend validator for crosstalk+finecalib for FIF #1053

sappelhoff · 2020-09-03T12:54:11Z

Here I added some tests that must pass --> i.e., as long as they don't, we need to change the validator.

@hoechenberger please look at my suggested "good" and "bad" filenames --> do you think this is fine when reading the specification here bids-standard/bids-specification#581 and when considering your experience?

I think it should be fair enough to allow people to store their crosstalk+finecalibration files:

at the root
for each subject / session

Can you think of other sensible places where to enable storage?

Also, can you think of other "bad filenames" (see code), if so, then we can add them to the tests to allow for better test driven development.

In any case, we should implement this part of the validator BEFORE the next release, so that if we find issues with the current spec for crosstalk+finecalib we can still adjust it without breakage issues.

cc @rwblair

PS: Everybody please feel free to push to this PR

EDIT: new related PR: bids-standard/bids-specification#598

sappelhoff · 2020-09-03T13:02:21Z

Note, if the current spec for these two files turns out to be a total pain, we could still make some adjustments to the spec -> https://github.com/bids-standard/bids-specification/pull/581/files

That is: we could constrain a bit more on how to store the files, if that makes our lives easier for the validation!

(For example, require that the two files MUST be stored on the subject/session/meg level)

bids-validator/tests/type.spec.js

hoechenberger · 2020-09-04T08:55:38Z

I think it should be fair enough to allow people to store their crosstalk+finecalibration files:

at the root

for each subject / session

Can you think of other sensible places where to enable storage?

I think your list covers all reasonable storage locations

hoechenberger · 2020-09-04T18:08:12Z

@sappelhoff I cannot push to your fork, could you grant me permission please?
You should see a checkbox on the right that looks sth like this:

sappelhoff · 2020-09-04T18:55:36Z

could you grant me permission please?

done, I gave you (temporary) maintainer permissions :-)

hoechenberger · 2020-09-04T18:58:11Z

done, I gave you (temporary) maintainer permissions :-)

Neat 🥳
Just pushed

Instead of extending the existing regexp and making it even harder to parse for a human brain, I simply added two new regexps for fine-calibration and cross-talk files

hoechenberger · 2020-09-04T19:23:53Z

Say… I just realized that modality-specific files like *_meg.* have to be placed in /sub-<label>/[ses-<label>/]meg/, according to https://bids-specification.readthedocs.io/en/stable/04-modality-specific-files/02-magnetoencephalography.html#meg-recording-data, is that correct? In that case, I can drop half of the changes I've made here…

sappelhoff · 2020-09-05T07:11:48Z

I just realized that modality-specific files like _meg. have to be placed in /sub-/[ses-/]meg/

yes, I think that's correct. @robertoostenveld can you weigh in with some advice on how you would store crosstalk+calibration files for FIF, please?

This looks a bit odd to me:

/sub-12/sub-12_acq-calibration_meg.fif

What I think we should make valid (just finecalib examples here, crosstalk should be the same):

acq-calibration_meg.fif ... define at the root ... valid for everybody and all sessions (--> how common is this case?! should we drop this?)
/sub-12/meg/sub-12_acq-calibration_meg.fif ... if no session folders are present (=there is only one session ... --> this boils down to 1xcalib and 1xcrosstalk per subj ... probably leads to the same files being duplicated at different subj levels, e.g., because subjs have been measured on the same day .. or even the same week/month/...?)
/sub-12/ses-01/meg/sub-12_ses-01_acq-calibration_meg.fif ... define separately for each session (--> e.g., if one subject has been measured one at time X and once at time X+6months)

I think what's missing from this and what you @hoechenberger wanted is

/sub-12/sub-12_acq-calibration_meg.fif ... store at subject level --> valid for all sessions of that subject

but that collides with the "meg" stuff should be stored in .../meg... (although arguably the acq-calibration_meg.fif defined at the root collides with that rule as well)

hoechenberger · 2020-09-05T07:39:31Z

acq-calibration_meg.fif ... define at the root ... valid for everybody and all sessions (--> how common is this case?! should we drop this?)

This is actually quite common, as these files typically only change with system service every 6 months or so. So if we drop this, we should do it for other reasons than "it's not a common use case" 😅

/sub-12/meg/sub-12_acq-calibration_meg.fif ... if no session folders are present (=there is only one session ... --> this boils down to 1xcalib and 1xcrosstalk per subj ... probably leads to the same files being duplicated at different subj levels, e.g., because subjs have been measured on the same day .. or even the same week/month/...?)

I don't understand this, actually. Assume I have a long-running study with only a single session for each participant, but with a large number of participants. In that case, it's possible that the files do change between participants.

/sub-12/ses-01/meg/sub-12_ses-01_acq-calibration_meg.fif ... define separately for each session (--> e.g., if one subject has been measured one at time X and once at time X+6months)

Yep, so this one is clear and crisp 😁

I think what's missing from this and what you @hoechenberger wanted is

/sub-12/sub-12_acq-calibration_meg.fif ... store at subject level --> valid for all sessions of that subject

Yeah, this is the one you pointed out as looking a little odd.

but that collides with the "meg" stuff should be stored in .../meg... (although arguably the acq-calibration_meg.fif defined at the root collides with that rule as well)

Yep.

I mean. To make our lives super easy, we could decide to only allow

/sub-12/meg/sub-12_acq-calibration_meg.fif
/sub-12/ses-01/meg/sub-12_ses-01_acq-calibration_meg.fif

But let's see what @robertoostenveld thinks :)

sappelhoff · 2020-09-05T07:48:31Z

acq_calibration_meg.fif (at root level)

This is actually quite common, as these files typically only change with system service every 6 months or so

I'd be fine to allow it :)

/sub-12/meg/sub-12_acq-calibration_meg.fif ... if no session folders are present (=there is only one session ... --> this boils down to 1xcalib and 1xcrosstalk per subj ... probably leads to the same files being duplicated at different subj levels, e.g., because subjs have been measured on the same day .. or even the same week/month/...?)

I don't understand this, actually. Assume I have a long-running study with only a single session for each participant, but with a large number of participants. In that case, it's possible that the files do change between participants.

this kind of syntax would also cover studies where the calib/crosstalk changes mid study:

/sub-12/meg/sub-12_acq-calibration_meg.fif
crosstalk/calib are changed through maintenance
/sub-13/meg/sub-13_acq-calibration_meg.fif ... the contents of this file are different from that of sub-12

To make our lives super easy, we could decide to only allow
/sub-12/meg/sub-12_acq-calibration_meg.fif
/sub-12/ses-01/meg/sub-12_ses-01_acq-calibration_meg.fif

True ... that's tempting as well.

However we decide, I am +1 on adjusting the spec from bids-standard/bids-specification#581 to include a more complete list of examples on how we want users to specify crosstalk + finecalibration

hoechenberger · 2020-09-05T11:33:31Z

However we decide, I am +1 on adjusting the spec from bids-standard/bids-specification#581 to include a more complete list of examples on how we want users to specify crosstalk + finecalibration

So now I gotta ask: what's the "BIDS philosophy" when addressing questions like these? I, personally, would be totally fine requiring these paths:

/sub-12/meg/sub-12_acq-calibration_meg.fif
/sub-12/ses-01/meg/sub-12_ses-01_acq-calibration_meg.fif

and not allowing anything else. Super clear, super restrictive. Would lead to some redundancy in most studies (storing the exact same files a bunch of times, for each session / participant)

But that's not how the BIDS specification is usually designed, right?

sappelhoff · 2020-09-05T14:40:40Z

Super clear, super restrictive.

would be fine with me in principle

Would lead to some redundancy in most studies (storing the exact same files a bunch of times, for each session / participant)

you see redundancy in a lot of places in BIDS ... and yet in other places you find the inheritance principle to reduce that redundancy.

In our case I would consider this kind of redundancy fine.

But that's not how the BIDS specification is usually designed, right?

Just a side note: BIDS is continuously evolving and growing, so its initial design (whatever it was) has long been overhauled, changed, extended --> that's one of BIDS's drawbacks and advantages: We can accommodate a lot and we can work with the community and adapt ... at the expense of sometimes being clunky and not as strict as other standards are.

hoechenberger · 2020-09-05T15:46:10Z

/sub-12/sub-12_acq-calibration_meg.fif ... store at subject level --> valid for all sessions of that subject

but that collides with the "meg" stuff should be stored in .../meg... (although arguably the acq-calibration_meg.fif defined at the root collides with that rule as well)

This dataset:
https://github.com/OpenNeuroDatasets/ds000248

also has a bunch of acq- files related to MRI recordings in the root, so I guess it would make sense to support this for MEG too.

hoechenberger · 2020-09-05T15:47:33Z

… and considering all of that. Maybe we can just leave things (in this PR) as they are, and only need to adjust the specs? To recap, the supported paths are:

/acq-calibration_meg.fif
/ses-01_acq-calibration_meg.fif
/sub-12/sub-12_acq-calibration_meg.fif <-- this one "feels" a little odd, but would apply to all sessions w/in a subject
/sub-12/meg/sub-12_acq-calibration_meg.fif
/sub-12/ses-01/meg/sub-12_ses-01_acq-calibration_meg.fif

sappelhoff · 2020-09-06T09:15:32Z

but they are .bidsignore..d --> https://github.com/OpenNeuroDatasets/ds000248/blob/a42fc3f4759ef6bda8253aea4e9403a0cc3a066d/.bidsignore#L1

😬

Over the course of this discussion and sleeping over it, I am now tending towards making our lives easy at first, and only allow:

/sub-12/meg/sub-12_acq-calibration_meg.fif
/sub-12/ses-01/meg/sub-12_ses-01_acq-calibration_meg.fif

Pros:

this "style" of saving MEG files is widely used and spec conform
crosstalk+finecalib can be stored for each participant/session combination --> there is no case where we cannot store the data
no need to have a new, far-reaching discussion with all kinds of BIDS stakeholders

Cons:

leads to duplication of files

Action items:

adjust this PR
adjust the spec to be more restrictive

Sunday morning thought:

Implementing the specification in the validator really ALWAYS lets you look upon the situation from a new angle ... and often reveals flaws ... that's a good lesson to avoid changes that you are not sure how to validate.

PS:

doing this "easy" way does not prevent us from extending it in the future in a backwards compatible way ... however, doing "too much" now may prove harmful.

hoechenberger · 2020-09-06T09:20:25Z

@sappelhoff I agree with everything you said

I will do the adjustments to this PR later today, and propose adjustments to the specs where needed.

We can always extend the specs later to include more storage places should it turn out to be required. Until then, maybe it's super clean to store all MEG stuff under meg/.

…files Based on discussion in https://github.com/bids-standard/bids-validator/pull/1053 Specifically see https://github.com/bids-standard/bids-validator/pull/1053#issuecomment-687735083

hoechenberger · 2020-09-06T14:26:52Z

@sappelhoff
Done, we're back to only allowing "file-level" paths, i.e.,

/sub-12/meg/sub-12_acq-calibration_meg.fif
/sub-12/ses-01/meg/sub-12_ses-01_acq-calibration_meg.fif

as discussed.

sappelhoff

awesome, I can't approve technically because GitHub counts this as my own PR, but I consider this +1

Thanks a bunch! ... but before merging, I want @rwblair to have a look at this and approve or suggest changes.

bids-validator/bids_validator/rules/file_level_rules.json

Keep ses match in capturing group, but wrap in a non capturing group that requires the trailing slash. Current regex has that slash as always optional, would allow a file name that started with ses-<label> like as well as allowing the valid ses-<label>/sub-<label>_ses-<label>_acq-calibration_meg.fif

hoechenberger · 2020-09-08T18:28:08Z

Awesome, thanks @rwblair and @sappelhoff!

sappelhoff · 2020-09-08T18:28:39Z

And thanks @hoechenberger as well :-)

robertoostenveld · 2020-09-08T19:48:23Z

I did not have time earlier, but just read up on this. The proposed solution under "meg" (for now) looks good to me and can always be improved/extended in the future.

Perhaps for future consideration is that this acq-calibration_meg.fif file might be compared to the bvec and bval files for dwi. Internal consistency is a general aspect of the "BIDS philosophy", so if a solution is defined for case 1, and case 2 is similar, the same solution should be adopted for 2 as well.

We accidentally mixed up the Elekta/Neuromag/MEGIN cross-talk and fine-calibration files' extensions, `.fif` and `.dat`, respectively, in bids-standard#1053.

We accidentally mixed up the Elekta/Neuromag/MEGIN cross-talk and fine-calibration files' extensions, `.fif` and`.dat`, respectively, in bids-standard#1053.

We accidentally mixed up the Elekta/Neuromag/MEGIN cross-talk and fine-calibration files' extensions, `.fif` and`.dat`, respectively, in #1053.

We accidentally mixed up the Elekta/Neuromag/MEGIN cross-talk and fine-calibration files' extensions, `.fif` and`.dat`, respectively, in bids-standard#1053.

sappelhoff added 2 commits September 3, 2020 14:49

add tests for crosstalk+finecalib for FIF

8acd326

fix linter

90d2419

hoechenberger reviewed Sep 4, 2020

View reviewed changes

bids-validator/tests/type.spec.js Outdated Show resolved Hide resolved

hoechenberger added 2 commits September 4, 2020 20:03

Add VS Code dir to .gitignore

34bd1e3

Validate MEG fine-calibration & cross-talk files

583ae84

hoechenberger added 2 commits September 4, 2020 20:59

Style

a0dd137

Style

a2830ac

More test strings

6f71fe1

sappelhoff mentioned this pull request Sep 6, 2020

[FIX] tighter rules for sharing MEG cross-talk and fine-calibration .fif files bids-standard/bids-specification#598

Merged

hoechenberger added 3 commits September 6, 2020 16:19

Remove support for session-level paths

49513d8

Remove support for subject-level paths

5cc795a

Remove support for top-level paths

1a2bd6e

Add bad session-level filenames to test

e1cc628

sappelhoff commented Sep 6, 2020

View reviewed changes

rwblair requested changes Sep 8, 2020

View reviewed changes

bids-validator/bids_validator/rules/file_level_rules.json Outdated Show resolved Hide resolved

hoechenberger and others added 2 commits September 8, 2020 17:46

Support lacking ses entity

acf479b

rwblair approved these changes Sep 8, 2020

View reviewed changes

rwblair merged commit a6d9ada into bids-standard:master Sep 8, 2020

sappelhoff deleted the crosstalk branch September 8, 2020 18:27

hoechenberger mentioned this pull request Sep 8, 2020

MEG fine-calibration and crosstalk files mne-tools/mne-bids#495

Closed

hoechenberger mentioned this pull request Sep 25, 2020

Fix cross-talk and fine-calibration extensions #1079

Merged

sappelhoff pushed a commit that referenced this pull request Sep 27, 2020

Fix cross-talk and fine-calibration extensions

9bf715d

We accidentally mixed up the Elekta/Neuromag/MEGIN cross-talk and fine-calibration files' extensions, `.fif` and`.dat`, respectively, in #1053.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend validator for crosstalk+finecalib for FIF #1053

Extend validator for crosstalk+finecalib for FIF #1053

sappelhoff commented Sep 3, 2020 •

edited

Loading

sappelhoff commented Sep 3, 2020

hoechenberger commented Sep 4, 2020

hoechenberger commented Sep 4, 2020

sappelhoff commented Sep 4, 2020

hoechenberger commented Sep 4, 2020

hoechenberger commented Sep 4, 2020 •

edited

Loading

sappelhoff commented Sep 5, 2020

hoechenberger commented Sep 5, 2020

sappelhoff commented Sep 5, 2020

hoechenberger commented Sep 5, 2020

sappelhoff commented Sep 5, 2020

hoechenberger commented Sep 5, 2020

hoechenberger commented Sep 5, 2020 •

edited

Loading

sappelhoff commented Sep 6, 2020 •

edited

Loading

hoechenberger commented Sep 6, 2020 •

edited

Loading

hoechenberger commented Sep 6, 2020

sappelhoff left a comment

hoechenberger commented Sep 8, 2020

sappelhoff commented Sep 8, 2020

robertoostenveld commented Sep 8, 2020

Extend validator for crosstalk+finecalib for FIF #1053

Extend validator for crosstalk+finecalib for FIF #1053

Conversation

sappelhoff commented Sep 3, 2020 • edited Loading

sappelhoff commented Sep 3, 2020

hoechenberger commented Sep 4, 2020

hoechenberger commented Sep 4, 2020

sappelhoff commented Sep 4, 2020

hoechenberger commented Sep 4, 2020

hoechenberger commented Sep 4, 2020 • edited Loading

sappelhoff commented Sep 5, 2020

hoechenberger commented Sep 5, 2020

sappelhoff commented Sep 5, 2020

hoechenberger commented Sep 5, 2020

sappelhoff commented Sep 5, 2020

hoechenberger commented Sep 5, 2020

hoechenberger commented Sep 5, 2020 • edited Loading

sappelhoff commented Sep 6, 2020 • edited Loading

hoechenberger commented Sep 6, 2020 • edited Loading

hoechenberger commented Sep 6, 2020

sappelhoff left a comment

Choose a reason for hiding this comment

hoechenberger commented Sep 8, 2020

sappelhoff commented Sep 8, 2020

robertoostenveld commented Sep 8, 2020

sappelhoff commented Sep 3, 2020 •

edited

Loading

hoechenberger commented Sep 4, 2020 •

edited

Loading

hoechenberger commented Sep 5, 2020 •

edited

Loading

sappelhoff commented Sep 6, 2020 •

edited

Loading

hoechenberger commented Sep 6, 2020 •

edited

Loading