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

BEP 012: Functional MRI derivatives #519

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

BEP 012: Functional MRI derivatives #519

wants to merge 14 commits into from

Conversation

effigies
Copy link
Collaborator

@effigies effigies commented Jun 30, 2020

This PR has been extracted from nipreps/fmriprep#207, following the incorporation of BEP 003 - Common Derivatives (#265).

I've gone through a first pass of bringing it into conformation with BEP 003. I think there are no direct conflicts, so I would like to open this up to community comment.

I think the @bids-standard/derivatives-electrophys team should have a look at the _timeseries.<ext> components. I suspect that at least some of this could be written more generally to ensure that we make BEP021 a natural extension rather than an awkward re-architecting.

Link HTML render of the BEP: https://bids-specification--519.org.readthedocs.build/en/519/derivatives/functional-derivatives.html

Moderators: @effigies @cmaumet

Comment on lines 130 to 131
| `_var_norm` | The time series has been variance normalized |
| `_centered` | The time series has had its mean subtracted |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would X_centered_var_norm then be a substitute for something like _z when z-scoring values? Or would users be more likely to create a new suffix for additional standard transformations like z-scoring?

Also, if the values are underscore-delimited, could they be converted to camel-case to make it easier to separate them (especially as new values are added)?

@tsalo
Copy link
Member

tsalo commented Aug 11, 2020

To continue the conversation from https://github.com/poldracklab/fmriprep/issues/2232#issuecomment-671653585:

@effigies said:

Regarding the decomposition.json, it looks like a table serialized to JSON, so I wonder why we wouldn't target TSVs. The derivatives spec, while a useful tool for standardizing common things, is inherently incomplete, and tools are allowed to produce unspecified derivatives. It makes more sense to me to spend a year or two with things in an unstable but useful state than wedging data into a specified but awkward structure.

@emdupre said:

I think we've generally aimed to build off of the fMRI derivatives BEP, which currently recommends a decomposition.json file specifically. Though I may have misunderstood how that file was originally envisioned !

Personally, I like having the metadata file in a JSON rather than a TSV, but that's only because it feels more "BIDS-y" to me. Though I'm certainly not the expert in this discussion on that 😸 All that to say, and seconding @\oesteban: Maybe we should have this conversation on that specific part of the derivatives BEP ?

Personally, I'd like to see these decomposition metrics and classifications supported in as BIDS Derivatives-compatible a manner as possible, since adding metadata to decompositions is very common, especially if you want to retain as much information as possible (e.g., variance explained for PCA components in CompCor, AROMA classifications, etc.). If a TSV targeting the decomposition JSON, with its own JSON describing the TSV's columns, would be the best way to do it, then could that be supported here?

@tsalo
Copy link
Member

tsalo commented Aug 18, 2020

Per maintainers call today, two possible routes forward for decomposition statistics are (1) house everything in jsons (as tedana currently does) or (2) add a new suffix for TSVs containing these stats (e.g., _decompStats.tsv). The former is completely valid under the current rules, and the latter can always be adopted later, so the consensus was to stick with a json file for the immediate future, for both fMRIPrep and the Functional Derivatives BEP.

## Functional derivatives maps

A derivative map is a measure derived from a functional series, mapped onto spatial
locations as defined by voxels in a volume or vertices on a surface.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these the only ways to define a location? If we have an electrode, for example, its localization can't be Cz, must be its coordinates, right? Are coordinates also included in this definition?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guiomar Sorry, this is extremely late response, but I'm not sure what to do with this. Is your suggestion that we use fMRI derivatives to describe source-localized EEG/MEG maps?

headers indicating the name of the time series.
In the case where every voxel has a time series (i.e., voxel-wise regressors,
as in ANATICOR), then the time series should be saved as a NIfTI file.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reminder link of available atlases

regions in the volume. An atlas may be three- or four-dimensional, which affects
the interpretation of the roi index.

ROI (optional) - For 3D atlases, the ROI label should be numeric, corresponding
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the examples encode the camel cased roi value in the column name. Does it make sense to duplicate this information in the ROI key in the sidecar?

@tsalo tsalo marked this pull request as ready for review August 22, 2022 17:30
@tsalo tsalo marked this pull request as draft August 22, 2022 17:30
@Remi-Gau
Copy link
Collaborator

maintenance note: added a link to the rendered BEP in the top message of this PR.

@codecov
Copy link

codecov bot commented Jun 28, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (6d7eb0f) 87.83% compared to head (1c79cbc) 87.83%.
Report is 37 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     nipreps/fmriprep#519   +/-   ##
=======================================
  Coverage   87.83%   87.83%           
=======================================
  Files          16       16           
  Lines        1356     1356           
=======================================
  Hits         1191     1191           
  Misses        165      165           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@effigies effigies changed the title BEP 012: Functional derivatives BEP 012: Functional MRI derivatives Jun 28, 2023

| Field name | Definition |
| :---------------- | :------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| SamplingFrequency | REQUIRED. Sampling frequency (in Hz) of all columns in the file. Special value `"TR"` indicates one sample per volume of a corresponding BOLD series. |

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the actual TR value should be saved in the metadata for distributing the derivative. Otherwise users will need to find the original dataset to get the actual TR for analysis.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The BOLD timing information should be associated with the BOLD file, whether raw or derived. I'm not sure the advantage of duplicating it in timeseries.tsv. What is your use case?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracting time series from ROIs from multiple datasets of different TR, and we potentially need to resample all the data to the same sampling rate for time series model (e.g. autoregressive model).

Copy link
Collaborator Author

@effigies effigies Aug 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, as there may not be an associated BOLD file. But in that case, would you not just set SamplingFrequency to 1/RepetitionTime? Along with StartTime, that's a complete definition.

We should consider the VolumeTiming case for BOLD. We could see if VolumeTiming's definition can accommodate an expansion along the lines of "In the case of ROI time series, the VolumeTiming is the timing information for the source BOLD series." Or we could create a SampleTiming array that makes it explicit. Or we follow PET's lead with blood recordings and add a time column in seconds.

I'm a little inclined toward the time column, as that would be a reasonable addition to any timeseries.tsv, and cover even and unevenly-spaced time series.

It would probably be good to have an explicit rule where timeseries with no associated BOLD series and no time column SHOULD NOT set "SamplingFrequency": "TR".

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

time column is the clearest way to me to include this piece of information. I like this idea.

@shnizzedy
Copy link

In light of #981, specifically the _motion suffix,

motion:
value: motion
display_name: Motion
description: |
Data fromrecorded from a tracking system store.
should
### Motion-related time series
Template:
```Text
<pipeline_name>/
sub-<label>/
func/
<source_entities>[_desc-<label>]_motion.tsv
<source_entities>[_desc-<label>]_motion.json
```
revert to _timeseries, change to something unambiguous, or will the sidecar and context be sufficient to differentiate the different _motion files?

@shnizzedy shnizzedy mentioned this pull request Oct 13, 2023
8 tasks
@effigies
Copy link
Collaborator Author

I guess we need to see if registration-derived motion parameters are too different to use the same suffix. I assumed we had a case not too dissimilar and we could treat these as the same type of data, kind of like how you could create synthetic bold series from a forward model, but I have not considered this in detail.

Comment on lines +275 to +278
| Column name | Units | Description |
| ------------------------------- | ------- | ---------------------- |
| `trans_x`, `trans_y`, `trans_z` | mm | Translation parameters |
| `rot_x`, `rot_y`, `rot_z` | radians | Rotation parameters |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Successfully merging this pull request may close these issues.

8 participants