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

[FIX] properly infer slice_time_ref from BIDS derivatives #3605

Merged
merged 50 commits into from Apr 12, 2023
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
97f0663
checks for tr and slice time
Remi-Gau Mar 18, 2023
fe4cbe7
improve getting t_r and slice_time_ref
Remi-Gau Mar 18, 2023
4e6a449
refactor
Remi-Gau Mar 18, 2023
b0c5477
refactor
Remi-Gau Mar 18, 2023
fbf556d
refactor
Remi-Gau Mar 18, 2023
80ff3cc
[DATALAD] Recorded changes
Remi-Gau Mar 18, 2023
4343da8
refactor
Remi-Gau Mar 18, 2023
79d33c4
refactor
Remi-Gau Mar 18, 2023
908a5cb
refactor
Remi-Gau Mar 18, 2023
c775545
fix
Remi-Gau Mar 18, 2023
94b1a4e
add more tests
Remi-Gau Mar 18, 2023
0a06581
fix
Remi-Gau Mar 18, 2023
843e8ef
use pytest fixture
Remi-Gau Mar 18, 2023
5a6a83b
add indent for readability
Remi-Gau Mar 18, 2023
15e43ad
update changelog
Remi-Gau Mar 20, 2023
3aecd4a
Merge branch 'main' into slice_timing
Remi-Gau Mar 22, 2023
158ca00
Apply suggestions from code review
Remi-Gau Mar 22, 2023
0a81c72
Merge remote-tracking branch 'upstream/main' into slice_timing
Remi-Gau Mar 29, 2023
06e3442
fix tests
Remi-Gau Mar 29, 2023
3534e12
add verbose level to reduce number of warnings
Remi-Gau Mar 29, 2023
0c42bd7
rm type annotations
Remi-Gau Mar 29, 2023
4910735
extend coverage
Remi-Gau Mar 29, 2023
058361b
Apply suggestions from code review
Remi-Gau Mar 30, 2023
b1ad2f5
[DATALAD] Recorded changes
Remi-Gau Mar 30, 2023
771e26a
rm type hints
Remi-Gau Mar 30, 2023
292846d
rm warnings
Remi-Gau Mar 30, 2023
52864d7
refactor get_bids_file
Remi-Gau Mar 30, 2023
f2be27f
make xfail strict
Remi-Gau Mar 30, 2023
475a626
fix typo
Remi-Gau Mar 30, 2023
0b6fa7c
infer metadata all the time
Remi-Gau Mar 31, 2023
b02cacb
swith back to original default
Remi-Gau Mar 31, 2023
9dac2d7
use new default in most tests to reduce number of warnings
Remi-Gau Mar 31, 2023
cee0957
Apply suggestions from code review
Remi-Gau Mar 31, 2023
532b6be
update deprecation version number
Remi-Gau Mar 31, 2023
80d8f94
update doc
Remi-Gau Mar 31, 2023
54dcf91
Merge remote-tracking branch 'upstream/main' into slice_timing
Remi-Gau Apr 4, 2023
03f25f9
add test for depreaction warning
Remi-Gau Apr 4, 2023
eeea48e
Merge remote-tracking branch 'upstream/main' into slice_timing
Remi-Gau Apr 5, 2023
9594588
Merge remote-tracking branch 'upstream/main' into slice_timing
Remi-Gau Apr 6, 2023
ef839c1
lint
Remi-Gau Apr 6, 2023
a69c8fd
lint
Remi-Gau Apr 6, 2023
4a4ae5c
add test for waning
Remi-Gau Apr 6, 2023
f9cfeba
lint
Remi-Gau Apr 6, 2023
8c2f51c
update doc
Remi-Gau Apr 7, 2023
1130eb1
update doc
Remi-Gau Apr 7, 2023
91eeacb
fix flake8
Remi-Gau Apr 7, 2023
3785882
add doc strings
Remi-Gau Apr 11, 2023
ea368fc
Update nilearn/_utils/tests/test_data_gen.py
Remi-Gau Apr 11, 2023
8155ec0
typo
Remi-Gau Apr 11, 2023
de4a2a9
Merge remote-tracking branch 'upstream/main' into slice_timing
Remi-Gau Apr 12, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion doc/changes/latest.rst
Expand Up @@ -12,9 +12,10 @@ NEW

Fixes
-----

- Change calculation of TR in :func:`~.glm.first_level.compute_regressor` to be more precise (:gh:`3362` by `Anne-Sophie Kieslinger`_)

- Improve how :func:`~.glm.first_level.first_level_from_bids` handles fetching slice timing metadata and add additional input validation (:gh:`3605` by `Rémi Gau`_).

- :func:`~nilearn.interfaces.fmriprep.load_confounds` can support searching preprocessed data in native space. (:gh:`3531` by `Hao-Ting Wang`_)

- Add correct "zscore_sample" strategy to ``signal._standardize`` which will replace the default "zscore" strategy in release 0.13 (:gh:`3474` by `Yasmin Mzayek`_).
Expand Down
19 changes: 19 additions & 0 deletions nilearn/_utils/data_gen.py
ymzayek marked this conversation as resolved.
Show resolved Hide resolved
@@ -1,10 +1,14 @@
"""
Data generation utilities
"""
from __future__ import annotations

import json
import os
import string

from pathlib import Path

import numpy as np
import pandas as pd
import scipy.linalg
Expand Down Expand Up @@ -744,6 +748,21 @@
return confounds


def add_metadata_to_bids_derivatives(bids_path: str | Path,
metadata: dict,
json_file: str = None) -> Path:
Remi-Gau marked this conversation as resolved.
Show resolved Hide resolved
if json_file is None:
Remi-Gau marked this conversation as resolved.
Show resolved Hide resolved
json_file = (Path(bids_path) / 'derivatives' / 'sub-01' / 'ses-01' /
'func' / 'sub-01_ses-01_task-main_run-01_bold.json')
else:
json_file = Path(bids_path) / json_file

Check warning on line 758 in nilearn/_utils/data_gen.py

View check run for this annotation

Codecov / codecov/patch

nilearn/_utils/data_gen.py#L758

Added line #L758 was not covered by tests

with open(json_file, 'w') as f:
json.dump(metadata, f)

return json_file


def create_fake_bids_dataset(base_dir='',
n_sub=10,
n_ses=2,
Expand Down
19 changes: 19 additions & 0 deletions nilearn/_utils/tests/test_data_gen.py
@@ -1,3 +1,7 @@
"""Test for data generation utilities."""

import json

import numpy as np
import pytest
from nilearn._utils.data_gen import (
Expand All @@ -8,6 +12,21 @@
)
from nilearn.image import get_data

from nilearn._utils.data_gen import add_metadata_to_bids_derivatives


def test_add_metadata_to_bids_derivatives(tmp_path):
# bare bone smoke test
target_dir = tmp_path / 'derivatives' / 'sub-01' / 'ses-01' / 'func'
target_dir.mkdir(parents=True)
json_file = add_metadata_to_bids_derivatives(bids_path=tmp_path,
metadata={"foo": "bar"})
Remi-Gau marked this conversation as resolved.
Show resolved Hide resolved
assert json_file.exists()
assert json_file.name == 'sub-01_ses-01_task-main_run-01_bold.json'
with open(json_file, 'r') as f:
metadata = json.load(f)
assert metadata == {"foo": "bar"}


@pytest.mark.parametrize("window", ["boxcar", "hamming"])
def test_generate_regions_ts_no_overlap(window):
Expand Down
115 changes: 73 additions & 42 deletions nilearn/glm/first_level/first_level.py
Expand Up @@ -6,7 +6,6 @@

"""
import glob
import json
import os
import sys
import time
Expand All @@ -19,7 +18,11 @@
from sklearn.base import clone
from sklearn.cluster import KMeans

from nilearn.interfaces.bids import get_bids_files, parse_bids_filename
from nilearn.interfaces.bids import (get_bids_files,
parse_bids_filename)
from nilearn.interfaces.bids.query import \
(_infer_slice_timing_start_time_from_dataset,
_infer_repetition_time_from_dataset)
from nilearn._utils import fill_doc
from nilearn._utils.glm import (_check_events_file_uses_tab_separators,
_check_run_tables, _check_run_sample_masks)
Expand Down Expand Up @@ -355,7 +358,11 @@
signal_scaling=0, noise_model='ar1', verbose=0, n_jobs=1,
minimize_memory=True, subject_label=None, random_state=None):
# design matrix parameters
if t_r is not None:
_check_repetition_time(t_r)
self.t_r = t_r
if slice_time_ref is not None:
_check_slice_time_ref(slice_time_ref)
self.slice_time_ref = slice_time_ref
self.hrf_model = hrf_model
self.drift_model = drift_model
Expand Down Expand Up @@ -793,9 +800,27 @@
return output


def _check_repetition_time(t_r):
if not isinstance(t_r, (float, int)):
Remi-Gau marked this conversation as resolved.
Show resolved Hide resolved
raise TypeError("'t_r' must be a float or an integer. "
f"Got {type(t_r)} instead.")
if t_r <= 0:
raise ValueError("'t_r' must be positive. "
f"Got {t_r} instead.")


def _check_slice_time_ref(slice_time_ref):
Remi-Gau marked this conversation as resolved.
Show resolved Hide resolved
if not isinstance(slice_time_ref, (float, int)):
raise TypeError("'slice_time_ref' must be a float or an integer. "
f"Got {type(slice_time_ref)} instead.")
if slice_time_ref < 0 or slice_time_ref > 1:
raise ValueError("'slice_time_ref' must be between 0 and 1. "
f"Got {slice_time_ref} instead.")


def first_level_from_bids(dataset_path, task_label, space_label=None,
sub_labels=None,
img_filters=None, t_r=None, slice_time_ref=0.,
img_filters=None, t_r=None, slice_time_ref=None,
hrf_model='glover', drift_model='cosine',
high_pass=.01, drift_order=1, fir_delays=[0],
min_onset=-24, mask_img=None,
Expand Down Expand Up @@ -901,49 +926,55 @@
if not os.path.exists(derivatives_path):
raise ValueError('derivatives folder does not exist in given dataset')

# Get acq specs for models. RepetitionTime and SliceTimingReference.
# Get acq specs for models.
# RepetitionTime and StartTime for slice timing.
# Throw warning if no bold.json is found
filters = [('task', task_label)]
for img_filter in img_filters:
if img_filter[0] in ['acq', 'rec', 'run']:
filters.append(img_filter)

Check warning on line 935 in nilearn/glm/first_level/first_level.py

View check run for this annotation

Codecov / codecov/patch

nilearn/glm/first_level/first_level.py#L935

Added line #L935 was not covered by tests

if t_r is not None:
warn('RepetitionTime given in model_init as %d' % t_r)
warn('slice_time_ref is %d percent of the repetition '
'time' % slice_time_ref)
_check_repetition_time(t_r)
warn("'RepetitionTime' given in model_init as {t_r}")
Remi-Gau marked this conversation as resolved.
Show resolved Hide resolved
else:
filters = [('task', task_label)]
for img_filter in img_filters:
if img_filter[0] in ['acq', 'rec', 'run']:
filters.append(img_filter)

img_specs = get_bids_files(derivatives_path, modality_folder='func',
file_tag='bold', file_type='json',
filters=filters)
# If we don't find the parameter information in the derivatives folder
# we try to search in the raw data folder
if not img_specs:
img_specs = get_bids_files(dataset_path, modality_folder='func',
file_tag='bold', file_type='json',
filters=filters)
if not img_specs:
warn('No bold.json found in derivatives folder or '
'in dataset folder. t_r can not be inferred and will need to'
' be set manually in the list of models, otherwise their fit'
' will throw an exception')
t_r = _infer_repetition_time_from_dataset(
bids_path=derivatives_path,
filters=filters)
# If the parameter information is not found in the derivatives folder,
# a search is done in the raw data folder.
if t_r is None:
t_r = _infer_repetition_time_from_dataset(
bids_path=dataset_path,
filters=filters)
if t_r is not None:
_check_repetition_time(t_r)
else:
warn("'t_r' not provided and cannot be inferred from metadata. "

Check warning on line 953 in nilearn/glm/first_level/first_level.py

View check run for this annotation

Codecov / codecov/patch

nilearn/glm/first_level/first_level.py#L953

Added line #L953 was not covered by tests
"It will need to be set manually in the list of models, "
"otherwise their fit will throw an exception.")
bthirion marked this conversation as resolved.
Show resolved Hide resolved

if slice_time_ref is not None:
_check_slice_time_ref(slice_time_ref)
warn("'slice_time_ref' given in model_init as {slice_time_ref}")
Copy link
Member

Choose a reason for hiding this comment

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

We should not warn here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Gonna try to make a small table about the different situations (input argument VS what metadata exist in which dataset) to clarify when to throw warnings or errors.

Copy link
Member

Choose a reason for hiding this comment

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

This refactoring isn't very useful if slice_time_ref default is not changed to None. If we do this, we need to deprecate it properly as @bthirion pointed out. In any case, given the explanation in the docstrings, this warning becomes redundant with the one under it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will remove the warning

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ymzayek see my long comment below #3605 (comment)

But there the option of not changing the API and keeping the default to 0 and let users actively pass slice_time_ref=None if they want to let nilearn try to infer it from the dataset.

In this case, then this refactoring could be useful if even if we did not change the API.

I am not saying I am in favor of not changing the API: just explaining why this bit of code could be useful even if we did not change it.

Copy link
Member

Choose a reason for hiding this comment

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

@Remi-Gau Yes I agree that if we don't have to change the API, better not. But this implementation will only infer the slice_time_ref if the user passes None, while the old behavior tries to infer it if the default, which is 0., is kept. So a user that doesn't change their code that uses the default option will now get 0. set as their slice_time_ref while they expect it to be inferred. Does that make sense? Or maybe I have the logic wrong. If what I'm saying follows, then we'd need to go through a deprecation cycle as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I am correct the old behavior was:

  • try to infer t_r AND slice_time_ref if and only if t_r = None is passed (which was the default)
  • if a slice_time_ref is found then it overrides ANY value passed as argument.

Making more tables to compare old and new behavior depending on datasets and arguments...

warn("'slice_time_ref' is {slice_time_ref} percent of the repetition "
'time')
else:
StartTime = _infer_slice_timing_start_time_from_dataset(
bids_path=derivatives_path,
filters=filters)
if StartTime is not None and t_r is not None:
assert(StartTime < t_r)
slice_time_ref = StartTime / t_r
else:
specs = json.load(open(img_specs[0], 'r'))
if 'RepetitionTime' in specs:
t_r = float(specs['RepetitionTime'])
else:
warn('RepetitionTime not found in file %s. t_r can not be '
'inferred and will need to be set manually in the '
'list of models. Otherwise their fit will throw an '
' exception' % img_specs[0])
if 'SliceTimingRef' in specs:
slice_time_ref = float(specs['SliceTimingRef'])
else:
warn('SliceTimingRef not found in file %s. It will be assumed'
' that the slice timing reference is 0.0 percent of the '
'repetition time. If it is not the case it will need to '
'be set manually in the generated list of models' %
img_specs[0])
warn(f"'slice_time_ref' not provided "
"and cannot be inferred from metadata."
"It will be assumed that the slice timing reference "
"is 0.0 percent of the repetition time. "
"If it is not the case it will need to "
"be set manually in the generated list of models.")
slice_time_ref = 0.0
_check_slice_time_ref(slice_time_ref)

# Infer subjects in dataset
if not sub_labels:
Expand Down