Update MEG regexps and testsuite to comply with father level specification #789

sappelhoff · 2019-06-21T12:07:07Z

EDIT --> I now take over all MEG data formats, not just KIT

In this PR I add to the MEG regexp so that _markers.<sqd, mrk>) can be accepted, as well as general MEG data files in the KIT format ending either with .sqd or .con. see the specification

This is BIDS since these PRs:

cc @monkeyman192

I furthermore update the yarn.lock file to include the hed validator, which has recently been added (but no to the lock file yet)

This is based on a use case, where the validator throws an error for a correct KIT BIDS dataset, see: mne-tools/mne-bids#209 (comment)

Interesting observations (i.e., I don't know what's happening and why)

the error based on the use case did only show up for the recent validator version 1.2.4 ... but not for previous versions. Yet, I cannot find any commit leading up to 1.2.4 that should have changed the way the validator looks at MEG files ...
for the MEG regexp, looking at this line: https://github.com/bids-standard/bids-validator/blob/e3c6e4635f077d4b01e7c13baee4a0cf44199c69/bids-validator/bids_validator/rules/file_level_rules.json#L203

--> when I added my addition: "\\.(?:sqd|con)" to the END of the line ... it did not work (i.e., did not resolve the use case error) ... however, adding it to the MIDDLE, or the START of the line did work - without breaking any tests.

works: ["\\.(?:sqd|con)", "\\.fif", "\\.ds"]
works: ["\\.fif", "\\.(?:sqd|con)", "\\.ds"]
does NOT work: ["\\.fif", "\\.ds", "\\.(?:sqd|con)"]

Can somebody shed some light on this?

codecov · 2019-06-21T12:12:10Z

Codecov Report

Merging #789 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #789   +/-   ##
=======================================
  Coverage   89.02%   89.02%           
=======================================
  Files          79       79           
  Lines        2296     2296           
  Branches      422      422           
=======================================
  Hits         2044     2044           
  Misses        216      216           
  Partials       36       36

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3c6e46...561fb2c. Read the comment docs.

codecov · 2019-06-21T12:12:11Z

Codecov Report

Merging #789 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #789   +/-   ##
=======================================
  Coverage   89.02%   89.02%           
=======================================
  Files          79       79           
  Lines        2296     2296           
  Branches      422      422           
=======================================
  Hits         2044     2044           
  Misses        216      216           
  Partials       36       36

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3c6e46...8cd2462. Read the comment docs.

jasmainak · 2019-06-21T14:43:11Z

Yet, I cannot find any commit leading up to 1.2.4 that should have changed the way the validator looks at MEG files ...

you can try "git bisect" on a local version of the validator to figure out what leads to the problem

jasmainak · 2019-06-21T14:45:11Z

Can somebody shed some light on this?

I usually use https://regex101.com/ for these kinds of things ... :-)

jasmainak

my usual comment. Can you add a test? ;-)

sappelhoff · 2019-06-21T14:46:49Z

I usually use https://regex101.com/ for these kinds of things ... :-)

me too ... but for that specific case, it's a JS array with several strings that just get inserted into the regexp --> so the order of items in this array should not make a difference

sappelhoff · 2019-06-21T15:12:29Z

you can try "git bisect" on a local version of the validator to figure out what leads to the problem

git bisect seems super cool! ... I couldn't use it properly though, for the following puzzling reason:

Using the LOCAL validator version ... the use case of test_kit ALWAYS fails (regardless of version)
- note, I was using git checkout 1.2.3 or git checkout 1.2.4
However, using the GLOBAL (npm i -g bids-validator@1.2.4) validator version, test_kit fails only for version 1.2.4, but not when doing npm i -g bids-validator@1.2.4 right before ...

jasmainak · 2019-06-21T15:42:04Z

sounds like some packaging issue, but I'm not a javascript expert ...

jasmainak · 2019-06-21T15:42:56Z

bids-validator/tests/type.spec.js

+    '/sub-01/ses-001/meg/sub-01_ses-001_markers.sqd', // KIT with removed father level directory
+    '/sub-01/ses-001/meg/sub-01_ses-001_markers.mrk', // KIT with removed father level directory
+    '/sub-01/ses-001/meg/sub-01_ses-001_meg.sqd', // KIT with removed father level directory
+    '/sub-01/ses-001/meg/sub-01_ses-001_meg.con', // KIT with removed father level directory


can you add an example to badFilenames? E.g., something with a father level directory should fail

Mmh currently the validator provides a wildcard for everything that is within a father level directory, see: https://github.com/bids-standard/bids-validator/blob/e3c6e4635f077d4b01e7c13baee4a0cf44199c69/bids-validator/bids_validator/rules/file_level_rules.json#L201

the final part is the relevant one: ?(_meg(@@@_meg_type_@@@\\/.*|\\/.*)|

it will try to match either an meg_type

these are defined below: "@@@_meg_type_@@@": ["\\.fif", "\\.ds"],

or it will match a father level directory with arbitrary contents: |\\/.*

for a test that you suggest I would have to touch this logic ... which could have unforeseen consequences.

Two options:

make an issue and let some regex/JS expert deal with this

I try to do it

I would say if the tests pass, you should be fairly confident of your change. Any unforeseen consequences can be dealt with in the future by adding more tests (and thus increasing your confidence even more in subsequent changes!)

What file formats still allow father level directories? It's only CTF with the *.ds format, right? There are a bunch of tests which need to be updated to reflect this, no? I'll comment on the relevant part.

or it will match a father level directory with arbitrary contents:

I seem to remember there was a reason for allowing arbitrary contents. See the discussion in my old PR here: https://github.com/bids-standard/bids-validator/pull/499

MEG file formats with father level according to the latest spec:

CTF

BTi/4D (why does this one have two names, BTW?)

The initial company in San Diego that commercially developed MEG was called "BTi", which is probably an abbreviation . Later they were renamed into "4-D Neuroimaging", which is usually abbreviated (by me) as "4D".

jasmainak · 2019-06-22T12:10:12Z

bids-validator/tests/type.spec.js

@@ -176,6 +176,10 @@ describe('utils.type.file.isMEG', function() {
    '/sub-01/ses-001/meg/sub-01_ses-001_task-rest_run-01_meg/sub-01_ses-001_task-rest_run-01_meg.raw.mhd',
    '/sub-01/ses-001/meg/sub-01_ses-001_task-rest_run-01_meg/xyz', // for e.g., BTi files
    '/sub-01/ses-001/meg/sub-01_ses-001_task-rest_run-01_meg/sub-01_ses-001_markers.sqd',


is this still a good file name?

I think that

/sub-01/ses-001/meg/sub-01_ses-001_task-rest_run-01_meg/abc.raw /sub-01/ses-001/meg/sub-01_ses-001_task-rest_run-01_meg/abc.raw.mhd /sub-01/ses-001/meg/sub-01_ses-001_task-rest_run-02_meg/def.sqd /sub-01/ses-001/meg/sub-01_ses-001_task-rest_run-03_meg/ghi

are all good names for directory+file. The reason for the father directory sub-01_ses-001_task-rest_run-01_meg is to not require changes to the actual file names. So the files underneath can be called whatever, as long as those are known MEG files and can be processed in the software of the original MEG system manufacturer (i.e. BTi, KIT, ITAB).

It would be good to determine the allowed file names and/or extensions for the different systems. I suspect that https://github.com/fieldtrip/fieldtrip/blob/master/fileio/ft_filetype.m has quite a few of them.

I agree with @robertoostenveld :

So the files underneath can be called whatever, as long as those are known MEG files and can be processed in the software of the original MEG system manufacturer [emphasis added by @sappelhoff]

--> I think what @jasmainak wants is that we add tests for exactly that: I.e., do NOT accept whatever file underneath ... but instead only accept those files underneath from which we know that they are correctly there. E.g., CTF and BTi/4D (these are the only MEG data formats that have a father format, see my comment)

I think I generally agree with what @jasmainak says, but I could also imagine this in a separate PR, because it goes beyond the scope of this PR, which was the following:

Allow that KIT data is specified without father level, because it does not need one anymore (according to latest spec)

Add tests that pass with KIT data specified without father level

agreed that we should keep the PR focussed on one aspect, which is KIT.

If you want to do it in another PR that's fine but my comment above was for the KIT .sqd file, not the others :) Let's raise an issue so we don't forget.

sappelhoff · 2019-06-24T17:27:52Z

@robertoostenveld @jasmainak

I ended up revamping the test suite for all MEG data. Please check if you agree with this.

Father levels are only present for

BTI
CTF

The other 4 file formats do NOT have a father level anymore:

FIF
KRISS
KIT
ITAB

jasmainak · 2019-06-24T17:39:07Z

I did not check if the file formats with or without father level are correct or not. But the tests look good to me

robertoostenveld · 2019-06-25T06:55:46Z

bids-validator/tests/type.spec.js

    '/sub-01/ses-001/meg/sub-01_ses-001_task-rest_run-01_channels.tsv',
+    // Father directory files are fine for some file formats:
+    // Father dir: CTF data with a .ds ... the contents within .ds are not checked
+    '/sub-01/ses-001/meg/sub-01_ses-001_task-rest_run-01_meg.ds/catch-alp-good-f.meg4',


the contents within the CTF .ds must be checked, otherwise it is not a valid dataset and standard software (e..g that of CTF itself) will not be able to read it.

I agree, but this is not something I break in this PR ... it's never been there :-)

Also, I have opened an issue on this several months ago: https://github.com/bids-standard/bids-validator/issues/692

sappelhoff · 2019-06-25T11:03:11Z

merging after approval by Mainak and Robert. Thanks for the reviews!

sappelhoff added 3 commits June 21, 2019 13:13

gitignore yarn error log

619e49b

add hed validator to yarn lock

dadf1b0

allow KIT markers for MEG regexp

561fb2c

sappelhoff requested review from robertoostenveld and jasmainak June 21, 2019 12:07

sappelhoff mentioned this pull request Jun 21, 2019

Reader: Auto handle raw.info['bads'] and use mne.Annotations instead of returning events mne-tools/mne-bids#209

Merged

7 tasks

sappelhoff mentioned this pull request Jun 21, 2019

mixed formatting and doc improvements mne-tools/mne-bids-pipeline#33

Merged

jasmainak reviewed Jun 21, 2019

View reviewed changes

add tests

801491f

jasmainak reviewed Jun 21, 2019

View reviewed changes

jasmainak reviewed Jun 22, 2019

View reviewed changes

sappelhoff changed the title ~~Update MEG regexp~~ Update MEG regexp to allow KIT data format without father-level Jun 24, 2019

add big test suite

5b6e9f4

sappelhoff changed the title ~~Update MEG regexp to allow KIT data format without father-level~~ Update MEG regexps and testsuite to comply with father level specification Jun 24, 2019

fix CTF regexp

4a65be7

robertoostenveld requested changes Jun 25, 2019

View reviewed changes

robertoostenveld approved these changes Jun 25, 2019

View reviewed changes

sappelhoff mentioned this pull request Jun 25, 2019

KRISS MEG data question bids-standard/bids-specification#253

Closed

add _digitizer.txt

8cd2462

sappelhoff merged commit 731b8ae into bids-standard:master Jun 25, 2019

sappelhoff deleted the add_hed_yarn branch June 25, 2019 11:03

sappelhoff mentioned this pull request Jul 20, 2023

MEG file format "KRISS" mentions *_digitizer.txt file (and suffix) which is not defined in the spec bids-standard/bids-specification#1560

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update MEG regexps and testsuite to comply with father level specification #789

Update MEG regexps and testsuite to comply with father level specification #789

sappelhoff commented Jun 21, 2019 •

edited

Loading

codecov bot commented Jun 21, 2019

codecov bot commented Jun 21, 2019 •

edited

Loading

jasmainak commented Jun 21, 2019

jasmainak commented Jun 21, 2019

jasmainak left a comment

sappelhoff commented Jun 21, 2019

sappelhoff commented Jun 21, 2019

jasmainak commented Jun 21, 2019

jasmainak Jun 21, 2019

sappelhoff Jun 21, 2019

jasmainak Jun 22, 2019

jasmainak Jun 22, 2019

sappelhoff Jun 24, 2019

robertoostenveld Jun 24, 2019

jasmainak Jun 22, 2019

robertoostenveld Jun 24, 2019

sappelhoff Jun 24, 2019

robertoostenveld Jun 24, 2019

jasmainak Jun 24, 2019

sappelhoff commented Jun 24, 2019

jasmainak commented Jun 24, 2019

robertoostenveld Jun 25, 2019

sappelhoff Jun 25, 2019

robertoostenveld Jun 25, 2019

sappelhoff commented Jun 25, 2019

Update MEG regexps and testsuite to comply with father level specification #789

Update MEG regexps and testsuite to comply with father level specification #789

Conversation

sappelhoff commented Jun 21, 2019 • edited Loading

Interesting observations (i.e., I don't know what's happening and why)

codecov bot commented Jun 21, 2019

Codecov Report

codecov bot commented Jun 21, 2019 • edited Loading

Codecov Report

jasmainak commented Jun 21, 2019

jasmainak commented Jun 21, 2019

jasmainak left a comment

Choose a reason for hiding this comment

sappelhoff commented Jun 21, 2019

sappelhoff commented Jun 21, 2019

jasmainak commented Jun 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sappelhoff commented Jun 24, 2019

jasmainak commented Jun 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sappelhoff commented Jun 25, 2019

sappelhoff commented Jun 21, 2019 •

edited

Loading

codecov bot commented Jun 21, 2019 •

edited

Loading