[MRG] ENH: Ignore entities from `derivatives` subfolder #470

hoechenberger · 2020-07-05T14:53:35Z

PR Description

When calling get_entity_vals(), we now ignore the derivatives folder in the BIDS root directory. This is important as the
derivatives may, for example, contain "mock" subjects like "sub-grand_average" or similar things, which we are typically
not interested in when looking at the BIDS root. If the users wants to process the data in derivatives/, they can just set the bids_root kwarg to bids_root/derivatives, and everything will work as expected.

The way I've coded the solution is not elegant at all, but it's simple and does do the trick…

Merge checklist

Maintainer, please confirm the following before merging:

All comments resolved
This is not your own PR
All CIs are happy
PR title starts with [MRG]
whats_new.rst is updated
PR description includes phrase "closes <#issue-number>"
Commit history does not contain any merge commits

When calling `get_entity_vals()`, we now ignore the `derivatives` folder in the BIDS root directory. This is important as the derivatives may, for example, contain "mock" subjects like "sub-grand_average" or similar things, which we are typically not interested in when looking at the BIDS root. If the users wants to process the data in `derivatives/`, they can just set the `bids_root` kwarg to `bids_root/derivatives`, and everything will work as expected.

hoechenberger · 2020-07-05T14:57:46Z

Forgot to add:
The reason I've opened this PR is because I was working on implementing "the sub-average idea" (#454 (comment)), and suddenly ran into issues when processing my data, as mne-study-template would try to read data from an average subject which only existed in derivatives/.

codecov-commenter · 2020-07-05T15:07:15Z

Codecov Report

Merging #470 into master will decrease coverage by 0.04%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master     #470      +/-   ##
==========================================
- Coverage   93.59%   93.54%   -0.05%     
==========================================
  Files          12       12              
  Lines        1749     1751       +2     
==========================================
+ Hits         1637     1638       +1     
- Misses        112      113       +1

Impacted Files	Coverage Δ
mne_bids/utils.py	`95.30% <50.00%> (-0.20%)`	⬇️

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 7d0cec0...72a308f. Read the comment docs.

sappelhoff

LGTM in principle, but we may have to adjust this in the future, once we start implementing derivatives also in MNE-BIDS.

hoechenberger · 2020-07-05T15:45:22Z

we may have to adjust this in the future, once we start implementing derivatives also in MNE-BIDS.

Absolutely!

hoechenberger · 2020-07-05T15:46:38Z

@sappelhoff An alternative could be to have get_entity_vals(..., include_derivatives=False)? Or even exclude_directories=..., but I'm not fully convinced myself yet!

sappelhoff · 2020-07-05T16:01:03Z

@sappelhoff An alternative could be to have get_entity_vals(..., include_derivatives=False)? Or even exclude_directories=..., but I'm not fully convinced myself yet!

these options sound at least more flexible, perhaps exclude_directories=None would be good? and then for your cases you just set it to ['derivatives']?

hoechenberger · 2020-07-05T16:05:35Z

these options sound at least more flexible, perhaps exclude_directories=None would be good? and then for your cases you just set it to ['derivatives']?

Yes this is what I was thinking. However I'm worried this could be a little "too" flexible, like the prefix kwarg in make_bids_basename, which can easily be abused… Do we need this kind of flexibility?

sappelhoff · 2020-07-05T16:13:27Z

Do we need this kind of flexibility?

not right now ... and when we do, we'll probably face challenges we cannot really anticipate right now 🤷‍♂️ I would be okay with not offering any flexibility ... but we should be aware that we may break the function in a couple of months (or a year)

hoechenberger · 2020-07-05T16:14:52Z

not right now ... and when we do, we'll probably face challenges we cannot really anticipate right now 🤷‍♂️ I would be okay with not offering any flexibility ... but we should be aware that we may break the function in a couple of months (or a year)

I guess that's alright for now… Will probably not be the only thing that breaks once we implement derivatives support!

That said, feel free to merge if you're happy ;)

sappelhoff

okay :-) thanks @hoechenberger

jasmainak · 2020-07-06T02:20:04Z

mne_bids/utils.py

+    Notes
+    -----
+    This function will scan the entire ``bids_root``, except for a
+    ``derivatives`` subfolder placed directly under ``bids_root``.


I'm sorry I'm late to the party. Why do you make this "exception" rather than look at .bidsignore ?

see: https://github.com/bids-standard/bids-validator#bidsignore. Unfortunately it's not mentioned in the specification :-( I think there was some disagreement whether there should be software specific ignore files or a general ignore file.

you probably mean bids-standard/bids-specification#131 @jasmainak

This sounds interesting! But as long as it's not part of the BIDS standard, … :/

whenever we have a use case, it may be worth reviving that discussion :-)

but big discussions like that (in BIDS) usually need at least one person who REALLY wants stuff to happen ... because otherwise it just goes stale ...

oh God yes, let's not go there again :-)

My personal opinion is that it's safe to exclude anything that's in .bidsignore because it has not been validated, and MNE-BIDS assumes that the data is BIDS-valid. Maybe other applications assume that full BIDS-compliance is not necessary and they can have .whateverignore. So we can locally take the decision without opening that big discussion again !

Yes but my dream is that ideally, the derivatives/ we create will be just another root of a valid BIDS dataset

that's probably already happening ... but I didn't follow very closely :)

yes, there is a big validator project to track the slow move to having BIDS-validated derivatives: https://github.com/bids-standard/bids-validator/projects/1

but note: it will always be possible to have "random" derivatives (unvalidated) as part of a BIDS raw directory ... only as soon as you put at dataset_description.json into the derivatives directory, it will get validated. (once we are there in the future)

sappelhoff reviewed Jul 5, 2020

View reviewed changes

hoechenberger mentioned this pull request Jul 5, 2020

[MRG] Treat grand-averaged data as if coming from "sub-average" & make derivatives naming more consistent mne-tools/mne-bids-pipeline#147

Merged

sappelhoff approved these changes Jul 5, 2020

View reviewed changes

sappelhoff changed the title ~~ENH: Ignore entities from derivatives subfolder~~ [MRG] ENH: Ignore entities from derivatives subfolder Jul 5, 2020

sappelhoff merged commit e0780da into mne-tools:master Jul 5, 2020

hoechenberger deleted the ignore-derivates-entities branch July 5, 2020 16:27

jasmainak reviewed Jul 6, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] ENH: Ignore entities from `derivatives` subfolder #470

[MRG] ENH: Ignore entities from `derivatives` subfolder #470

hoechenberger commented Jul 5, 2020 •

edited by sappelhoff

Loading

hoechenberger commented Jul 5, 2020

codecov-commenter commented Jul 5, 2020 •

edited

Loading

sappelhoff left a comment

hoechenberger commented Jul 5, 2020

hoechenberger commented Jul 5, 2020 •

edited

Loading

sappelhoff commented Jul 5, 2020

hoechenberger commented Jul 5, 2020

sappelhoff commented Jul 5, 2020

hoechenberger commented Jul 5, 2020

sappelhoff left a comment

jasmainak Jul 6, 2020

jasmainak Jul 6, 2020

sappelhoff Jul 6, 2020

hoechenberger Jul 6, 2020

sappelhoff Jul 6, 2020

jasmainak Jul 7, 2020

hoechenberger Jul 7, 2020

jasmainak Jul 9, 2020

sappelhoff Jul 9, 2020

[MRG] ENH: Ignore entities from derivatives subfolder #470

[MRG] ENH: Ignore entities from derivatives subfolder #470

Conversation

hoechenberger commented Jul 5, 2020 • edited by sappelhoff Loading

PR Description

Merge checklist

hoechenberger commented Jul 5, 2020

codecov-commenter commented Jul 5, 2020 • edited Loading

Codecov Report

sappelhoff left a comment

Choose a reason for hiding this comment

hoechenberger commented Jul 5, 2020

hoechenberger commented Jul 5, 2020 • edited Loading

sappelhoff commented Jul 5, 2020

hoechenberger commented Jul 5, 2020

sappelhoff commented Jul 5, 2020

hoechenberger commented Jul 5, 2020

sappelhoff left a 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

[MRG] ENH: Ignore entities from `derivatives` subfolder #470

[MRG] ENH: Ignore entities from `derivatives` subfolder #470

hoechenberger commented Jul 5, 2020 •

edited by sappelhoff

Loading

codecov-commenter commented Jul 5, 2020 •

edited

Loading

hoechenberger commented Jul 5, 2020 •

edited

Loading