Skip to content
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

read_raw_bids cannot use inherited metadata information #804

Open
kimsin98 opened this issue Jun 7, 2021 · 12 comments
Open

read_raw_bids cannot use inherited metadata information #804

kimsin98 opened this issue Jun 7, 2021 · 12 comments

Comments

@kimsin98
Copy link
Contributor

kimsin98 commented Jun 7, 2021

Describe the bug

The BIDS inheritance principle states that any metadata file can be defined at any directory level, and unless overwritten, lower levels inherit values from the top. However, read_raw_bids cannot read inherited metadata files.

Steps to reproduce

read_raw_bids on any subject EEG in the BIDS specification's first example EEG dataset, which has sidecar task-matchingpennies_eeg.json at top level.

Expected results

read_raw_bids searches higher directories when metadata files are not found at current level.

Actual results

RuntimeWarning: Did not find any eeg.json associated with sub-05_task-matchingpennies.

The search_str was "eeg_matchingpennies/sub-05/**/sub-05*eeg.json"
@hoechenberger
Copy link
Member

hoechenberger commented Jun 7, 2021

Thanks for reporting this!

The inheritance principle is really causing me headaches, I don't like it at all. But yes, seems we'll have to get this fixed. @sappelhoff any thoughts on how to approach this? This will be really painful to get right for all cases. Especially since we'll need to implement some sort of information merging between different levels of the BIDS hierarchy.

cc @agramfort

@sappelhoff
Copy link
Member

Yes, I don't particularly like the inheritance principle either. There have been several calls to formalize it "better" (or formalize in the first place), see e.g., bids-standard/bids-specification#102

For the sake of mne-bids, I agree with your assessment @hoechenberger ... getting started on this here will be bound to lead us towards having to formalize it within the bids spec.

As an immediate action it may be good to explicitly say in the docs that the inheritance principle is currently not supported.

@kimsin98
Copy link
Contributor Author

kimsin98 commented Jun 8, 2021

Yes, I don't particularly like the inheritance principle either. There have been several calls to formalize it "better" (or formalize in the first place), see e.g., bids-standard/bids-specification#102

Inheritance does seem quite messy after reading that issue. Some even propose removing it entirely in BIDS 2 (bids-standard/bids-2-devel#36). For now, what could be done is

  1. Query for all JSONs that apply to a specific file (all tags apply to the file), starting from top level and descending to the file level.
  2. Build a dict by reading the JSONs in that order. Later files may override values from earlier.
  3. Apply dict to Raw.

This is similar to how bids-standard/pybids handles inheritance (using PyBIDS is also an option).

@jasmainak
Copy link
Member

jasmainak commented Jun 8, 2021

@Aksoo it sounds like you've spent some time thinking about this. How much work would it be to start a pull request with a prototype?

@kimsin98
Copy link
Contributor Author

kimsin98 commented Jun 8, 2021

@Aksoo it sounds like you've spent some time thinking about this. How much work would it be to start a pull request with a prototype?

Depends on whether you are willing to add PyBIDS as dependency, but I am not sure I will have time until near the end of this month.

@jasmainak
Copy link
Member

I'd say ideally without PyBIDS dependency but @hoechenberger is in charge. Why is it easier with PyBIDS? Can you link to specific functions?

@kimsin98
Copy link
Contributor Author

kimsin98 commented Jun 8, 2021

I'd say ideally without PyBIDS dependency but @hoechenberger is in charge. Why is it easier with PyBIDS? Can you link to specific functions?

https://bids-standard.github.io/pybids/generated/bids.layout.BIDSLayout.html#bids.layout.BIDSLayout.get_metadata

@hoechenberger
Copy link
Member

I'd say ideally without PyBIDS dependency but @hoechenberger is in charge.

I'd say @sappelhoff is in charge 😅 My personal opinion about dependencies is: if they make our life easier and they are well-maintained, I see no reason not to add and use them. Or actually, it would be a waste of our time not to use them.

@sappelhoff
Copy link
Member

I'd say @sappelhoff is in charge 😅

😅

Let's just continue our mixture of consensus-based decision making and do-ocracy, so far it seems to work.

if they make our life easier and they are well-maintained, I see no reason not to add and use them.

I generally agree with that sentiment. It just means that instead of working a little harder for the feature ourselves (without adding a dependency), we are increasing the probability that at some point we'll have to make one or the other PR to pybids.

For our specific case, I am not sure whether the inheritance functionality could not be implemented in an inhertitance.py module just within mne-bids, that we extend bit by bit. Start with supporting the easiest cases ... and do the most complicated ones after it has actually been solved in the spec (bids-standard/bids-specification#102)

One more note on pybids: Tal Yarkoni, who initiated the package and wrote+rewrote large parts of it has just now changed jobs and will probably not be able to maintain it in the future. There is maybe going to be a discussion on this in the OHBM open science room. But that's just something to keep in mind when we think about adding it as a dependency.

These are the secondary dependencies we'd be adding:

https://github.com/bids-standard/pybids/blob/ec4b6f89670def3891e9cb4e030efdf84efdce0d/setup.cfg#L24-L33

@hoechenberger
Copy link
Member

For our specific case, I am not sure whether the inheritance functionality could not be implemented in an inhertitance.py module just within mne-bids, that we extend bit by bit. Start with supporting the easiest cases

I agree, let's try to do that and see how far we get without pybids for now :)

@jasmainak
Copy link
Member

yes, the secondary dependencies are what I worry about most. Sometimes, well-maintained packages don't control their dependencies very well and it leads to issues. PyBIDS seems to have come a long way ... their documentation has improved quite a bit and the dependency on Grabbit is gone.

@gael-vanderlee
Copy link

I'm running into the same issue, any update on this ? I am open to contributing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants