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

Expanding derivatives' configuration to validate existing BIDS-apps #881

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

GalKepler
Copy link
Contributor

Under the assumption that the main goal of this configuration file is to be able to query BIDS-apps' outputs, I think it would be great to validate the correspondence between the described deafult_path_patterns (and entities) and the actual outputs of these apps.

I've added some configurations that make it possible to query QSIPrep's derivatives, but I believe that people more familiar with possible outputs of this (and other) BIDS-app(s) will be able to make this even more robust.

@Remi-Gau
Copy link
Contributor

Remi-Gau commented Aug 3, 2022

Thanks for the PR and joining me in the "joyful" process of editing those config file manually.

So I think the assumption is partly right.

The config can help parse the outputs of bids Apps but only for the entities and filename patterns that are part of the bids specs.
So far the content of the config is limited to that (actually a subset of what is in the spec - see below).
So I think some of that PR goes beyond what we would want to have in those config.

Another reason why we may not want to add some of those changes is that we ultimately want to be able to automatically generate those config from the bids schema: when that happens that would destroy all the hard manual work of creating a config that works with bids apps that create outputs not supported by the bids schema.

That being said...

All the segmentation derivatives that are in the spec are not in the config and those should go in. I have been meaning to work on those for too long actually.
But it would be better if we added some tests to make make sure the configs are correct. Most likely using the fmriprep dataset from the bids examples.

Another thing that could be nice is to have a separate config file for a given bids app so that users can easily load that config.

But I am not sure if it makes sense to have them shipped with pybids: maybe a separate repo in the bids organization?

@VisLab
Copy link
Member

VisLab commented Aug 3, 2022 via email

@Remi-Gau
Copy link
Contributor

Remi-Gau commented Aug 3, 2022

Yeah I am having some thoughts to maybe move towards using configurations for bids matlab so that would be useful for that reason too.

@GalKepler
Copy link
Contributor Author

Thank you for the prompt reply!

Thanks for the PR and joining me in the "joyful" process of editing those config file manually.

So I think the assumption is partly right.

The config can help parse the outputs of bids Apps but only for the entities and filename patterns that are part of the bids specs. So far the content of the config is limited to that (actually a subset of what is in the spec - see below). So I think some of that PR goes beyond what we would want to have in those config.

Another reason why we may not want to add some of those changes is that we ultimately want to be able to automatically generate those config from the bids schema: when that happens that would destroy all the hard manual work of creating a config that works with bids apps that create outputs not supported by the bids schema.

That actually sounds great, I can understand why this PR is therefore somewhat irrelevant. Would love to contribute to the process of automating these path patterns 😄

That being said...

All the segmentation derivatives that are in the spec are not in the config and those should go in. I have been meaning to work on those for too long actually. But it would be better if we added some tests to make make sure the configs are correct. Most likely using the fmriprep dataset from the bids examples.

On it!

Another thing that could be nice is to have a separate config file for a given bids app so that users can easily load that config.

But I am not sure if it makes sense to have them shipped with pybids: maybe a separate repo in the bids organization?

I would encourage consideration of a separate repository for BIDS Apps support and for BIDS Transformations as well. BIDS Apps don't have to be in python do they? Neither do BIDS Transformations. A more language-independent approach would be useful.

@Remi-Gau and @VisLab, funny you should mention this, I've started working on such a repo (although it's python-focused), but it's still in a very early stage. Would love to join forces if it's something that seems beneficial to you.

@Remi-Gau
Copy link
Contributor

Remi-Gau commented Aug 4, 2022

@Remi-Gau and @VisLab, funny you should mention this, I've started working on such a repo (although it's python-focused), but it's still in a very early stage. Would love to join forces if it's something that seems beneficial to you.

Let's chat when I am back from holidays but I think that having a repo where people can submit config to help work with some bids apps could be of great value for the community.

@codecov
Copy link

codecov bot commented Aug 4, 2022

Codecov Report

Merging #881 (fd903a1) into master (1fa8382) will not change coverage.
The diff coverage is n/a.

❗ Current head fd903a1 differs from pull request most recent head be57b4d. Consider uploading reports for the commit be57b4d to get more accurate results

@@           Coverage Diff           @@
##           master     #881   +/-   ##
=======================================
  Coverage   86.44%   86.44%           
=======================================
  Files          35       35           
  Lines        3992     3992           
  Branches     1038     1038           
=======================================
  Hits         3451     3451           
  Misses        350      350           
  Partials      191      191           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Remi-Gau
Copy link
Contributor

Remi-Gau commented Aug 4, 2022

@GalBenZvi

If you want to look at the tests for new configs on bids examples datasets

The following tests that indexes some datasets from bids examples,

# Values for the number of file got from a:

@GalKepler
Copy link
Contributor Author

Let's chat when I am back from holidays but I think that having a repo where people can submit config to help work with some bids apps could be of great value for the community.

Sure, have fun!

@GalBenZvi

If you want to look at the tests for new configs on bids examples datasets

The following tests that indexes some datasets from bids examples,

# Values for the number of file got from a:

I'm actually kind of lost here... It seems that the current configuration isn't able to reconstruct paths of fmriprep's outputs.
I was thinking about parsing these outputs to their entities and then trying reconstructing the origin paths as a test for both the entities and default path pattern, but it doesn't seem like it's currently possible.

Obviously, a way to work around it is editing the configuration to capture these outputs, but I'm not sure if it's the right way to go following your notes.

@Remi-Gau
Copy link
Contributor

Remi-Gau commented Aug 5, 2022

I'm actually kind of lost here... It seems that the current configuration isn't able to reconstruct paths of fmriprep's outputs.\nI was thinking about parsing these outputs to their entities and then trying reconstructing the origin paths as a test for both the entities and default path pattern, but it doesn't seem like it's currently possible.

Sorry. I think I was unclear.

You are correct and that's because some files from fmriprep use entities that are yet part of the official spec.

So the idea would be to test the path builing only of the files that contain entities present in the spec. And skip the others.

This would also mean that one of the "low" hanging fruit for a config in the other repo you were mentioning would be to add an config for fmriprep that covers all the files it can generate.

@Remi-Gau
Copy link
Contributor

Remi-Gau commented Aug 5, 2022

here is how the "skipping of non covered entities" is done in another test

if entities["suffix"] in {"timeseries"}: # BEP012

@adelavega adelavega marked this pull request as draft September 19, 2022 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants