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

[ENH] Storing basic provenance on JSON sidecars of derivatives #300

Conversation

oesteban
Copy link
Collaborator

@oesteban oesteban commented Aug 13, 2019

Summary

In the context of BEP 016 we found ourselves prescribing what metadata fields should/could be included within derivatives. We then realized that some of them would easily extend to other Imaging derivatives (see #310), and also tapped on the quite controversial SkullStripped metadata field.

Proposal

Since BIDS has always been reluctant to include much provenance information, this PR proposes to make all these metadata fields OPTIONAL, and provides some suggestions for naming.

Original Content of this PR

Mapped from @Lestropie's bids-standard/bids-bep016#5
Closes bids-standard/bids-bep016#2

I have removed some keys that I believe should remain in BEP16 for being
applicable to dwi only.

I have also changed FieldInhomogeneity* with
SusceptibilityDistortion* since I think the latter will be more
familiar to a wider audience.

EDIT: fields that remained in BEP16 are - EddyCurrentCorrection, GradientNonLinearityQSpaceCorrection, SliceDropoutDetection, SliceDropoutReplacement

…fields from BEP16

Mapped from @Lestropie's bids-standard/bids-bep016#5
Closes bids-standard/bids-bep016#2

I have removed some keys that I believe should remain in BEP16 for being
applicable to dwi only.

I have also changed _FieldInhomogeneity*_ with
_SusceptibilityDistortion*_ since I think the latter will be more
familiar to a wider audience.
@oesteban
Copy link
Collaborator Author

cc/ @bids-standard/derivatives-mri-dwi

@franklin-feingold franklin-feingold added this to the 1.3.0 milestone Aug 14, 2019
@effigies
Copy link
Collaborator

Since this is common derivatives, we should be pinging @bids-standard/derivatives.

These questions in particular are relevant to @bids-standard/derivatives-mri, but there has been discussion (apologies for not linking at the moment; I'll try to find it) with @bids-standard/derivatives-electrophys that SkullStripped is problematic, as it only applies to MRI derivatives. That seems to be true here, as well, although at least they are optional.

I guess I would say that either we should merge this quickly and address the MRI-centricity in #265, or address it there first, and hold off on this until that's resolved.

@robertoostenveld
Copy link
Collaborator

SkullStripped but also FieldInhomogeneity etc. would never apply to MEG, EEG, iEEG, but also not to BEH, which is also a full-featured (but probably often overlooked) BIDS data type.

I don't think that any datatype specific metadata fields should be required as per common-derivatives.

@oesteban
Copy link
Collaborator Author

I drafted this PR to move out the discussion from BEP16, as we felt we were talking about metadata that could easily be present in other MRI derivatives.

That said, my opinion is that we should remove all these metadata bits for two reasons:

  • They do not apply to non-MRI data
  • They keep track of provenance, which is something beyond the scope of BIDS. Although it is difficult to keep provenance away in all the cases, I think in this particular time Skullstripped and the ones I added in this PR should be removed. IMHO it is a task for the workflow developers to allocate a dedicated provenance tracking (NIDM?) and/or insert the sufficient metadata information in more openly-scoped metadata fields/ READMEs / documentation / citation boilerplates.

I guess I would say that either we should merge this quickly and address the MRI-centricity in #265, or address it there first, and hold off on this until that's resolved.

I'm fine either way, but if Skullstripped is going away or a better solution is found in #265, then this PR does not make any sense. So I think I'm inclined to figure this out in #265 first and then downstream the decision here and into BEP16 (because we are defining there more of these metadata tracking keys - the non MRI side of it does not apply anymore, but the provenance argument is still valid).

@oesteban oesteban mentioned this pull request Aug 15, 2019
5 tasks
@oesteban oesteban changed the title [ENH] Upstream common-derivatives additional (optional) reserved JSON fields from BEP16 [ENH] Storing basic provenance on JSON sidecars of derivatives Sep 26, 2019
@sappelhoff
Copy link
Member

@oesteban this PR is still marked for the common derivatives milestone.

How do you want to proceed with this PR? Can it be closed? Should it become an issue for further discussion? Some other idea?

@satra
Copy link
Collaborator

satra commented Jun 2, 2020

@oesteban - i do think there is a need to curate a vocabulary around processes. perhaps we can work on that as a separate PR after the common derivatives is merged. as this vocabulary could be used in the provenance BEP #487 .

@sappelhoff sappelhoff removed this from the Common Derivatives milestone Jun 7, 2020
@oesteban
Copy link
Collaborator Author

Sure, what's the course of action regarding BEP016? Should we just remove mentions to these provenance entries?

@oesteban oesteban closed this Jun 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants