[FIX] properly infer slice_time_ref from BIDS derivatives #3605

Remi-Gau · 2023-03-18T19:46:24Z

Changes proposed in this pull request:

decouple fetching RepetitionTime from fetching metadata related to slice timing
start a deprecating cycle to change the default value of slice_time_ref from 0 to None
add t_r and slice_time_ref validation for first level models
extract functions to get metadata from bids sidecar json files
add appropriate tests including regression test and test for deprecation warning

github-actions · 2023-03-18T19:46:36Z

👋 @Remi-Gau Thanks for creating a PR!

Until this PR is ready for review, you can include the [WIP] tag in its title, or leave it as a github draft.

Please make sure it is compliant with our contributing guidelines. In particular, be sure it checks the boxes listed below.

PR has an interpretable title.
PR links to Github issue with mention Closes #XXXX (see our documentation on PR structure)
Code is PEP8-compliant (see our documentation on coding style)
Changelog or what's new entry in doc/changes/latest.rst (see our documentation on PR structure)

For new features:

There is at least one unit test per new function / class (see our documentation on testing)
The new feature is demoed in at least one relevant example.

For bug fixes:

There is at least one test that would fail under the original bug conditions.

We will review it as quick as possible, feel free to ping us with questions if needed.

codecov · 2023-03-18T19:57:50Z

Codecov Report

Merging #3605 (de4a2a9) into main (0df241e) will increase coverage by 0.08%.
The diff coverage is 97.77%.

@@            Coverage Diff             @@
##             main    #3605      +/-   ##
==========================================
+ Coverage   91.32%   91.41%   +0.08%     
==========================================
  Files         133      133              
  Lines       15475    15540      +65     
  Branches     3208     3228      +20     
==========================================
+ Hits        14133    14206      +73     
+ Misses        789      784       -5     
+ Partials      553      550       -3

Impacted Files	Coverage Δ
nilearn/glm/first_level/first_level.py	`93.80% <95.45%> (+2.15%)`	⬆️
nilearn/_utils/data_gen.py	`96.54% <100.00%> (+0.06%)`	⬆️
nilearn/interfaces/bids/_utils.py	`97.72% <100.00%> (ø)`
nilearn/interfaces/bids/query.py	`97.10% <100.00%> (+2.81%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

nilearn/_utils/data_gen.py

Co-authored-by: Taylor Salo <tsalo90@gmail.com>

nilearn/glm/first_level/first_level.py

Remi-Gau · 2023-03-29T14:56:55Z

nilearn/glm/first_level/first_level.py

@@ -1122,6 +1164,7 @@ def _get_processed_imgs(
        supported_filters=_bids_entities()["raw"]
        + _bids_entities()["derivatives"],
        extra_filter=img_filters,
+        verbose=verbose,


I am having to pass this verbose parameter around a bit too much to be able to control whether warnings, messages should be printed...

This should probably be handled better by implementing a logger in another PR.

Remi-Gau · 2023-03-31T13:18:45Z

then in this (perhaps edge) case, the behavior would be different

yup your comment just prompted me to actually try to map most cases to get a better idea because I was not even clear about it myself.

Remi-Gau · 2023-03-31T17:28:10Z

Todo

add test to specifically catch the deprecation warning.

nilearn/_utils/data_gen.py

nilearn/_utils/tests/test_data_gen.py

nilearn/glm/first_level/first_level.py

nilearn/glm/tests/test_first_level.py

nilearn/interfaces/tests/test_bids.py

bthirion

Almost there ! Thx

nilearn/glm/first_level/first_level.py

nilearn/interfaces/tests/test_bids.py

ymzayek

LGTM thx @Remi-Gau !

nilearn/_utils/tests/test_data_gen.py

Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>

bthirion

LGTM, thx.

ymzayek · 2023-04-12T07:50:59Z

@Remi-Gau can you rebase on main and resolve the conflict? Then this can be merged.

Remi-Gau · 2023-04-12T09:21:18Z

rebased + green CI = merging

Remi-Gau added 13 commits March 18, 2023 13:52

checks for tr and slice time

97f0663

improve getting t_r and slice_time_ref

fe4cbe7

refactor

4e6a449

refactor

b0c5477

refactor

fbf556d

[DATALAD] Recorded changes

80ff3cc

refactor

4343da8

refactor

79d33c4

refactor

908a5cb

fix

c775545

add more tests

94b1a4e

fix

0a06581

use pytest fixture

843e8ef

add indent for readability

5a6a83b

Remi-Gau mentioned this pull request Mar 20, 2023

[FIX] extend supported BIDS entities #3525

Merged

update changelog

15e43ad

Remi-Gau mentioned this pull request Mar 22, 2023

first_level_from_bids warnings ignore slice_time_ref argument #3299

Closed

Merge branch 'main' into slice_timing

3aecd4a

tsalo reviewed Mar 22, 2023

View reviewed changes

nilearn/_utils/data_gen.py Outdated Show resolved Hide resolved

Remi-Gau and others added 4 commits March 22, 2023 18:43

Apply suggestions from code review

158ca00

Co-authored-by: Taylor Salo <tsalo90@gmail.com>

Merge remote-tracking branch 'upstream/main' into slice_timing

0a81c72

fix tests

06e3442

add verbose level to reduce number of warnings

3534e12

Remi-Gau commented Mar 29, 2023

View reviewed changes

nilearn/glm/first_level/first_level.py Outdated Show resolved Hide resolved

Remi-Gau commented Mar 29, 2023

View reviewed changes

Remi-Gau added 2 commits March 29, 2023 17:07

rm type annotations

0c42bd7

extend coverage

4910735

Remi-Gau requested a review from ymzayek March 29, 2023 15:22

Remi-Gau added 2 commits March 31, 2023 15:20

update deprecation version number

532b6be

update doc

80d8f94

Remi-Gau added 6 commits April 4, 2023 10:50

Merge remote-tracking branch 'upstream/main' into slice_timing

54dcf91

add test for depreaction warning

03f25f9

Merge remote-tracking branch 'upstream/main' into slice_timing

eeea48e

Merge remote-tracking branch 'upstream/main' into slice_timing

9594588

lint

ef839c1

lint

a69c8fd

Remi-Gau requested review from ymzayek and bthirion April 6, 2023 12:41

Remi-Gau added 2 commits April 6, 2023 14:52

add test for waning

4a4ae5c

lint

f9cfeba

bthirion reviewed Apr 6, 2023

View reviewed changes

Remi-Gau added 3 commits April 7, 2023 14:11

update doc

8c2f51c

update doc

1130eb1

fix flake8

91eeacb

bthirion reviewed Apr 10, 2023

View reviewed changes

nilearn/glm/first_level/first_level.py Show resolved Hide resolved

nilearn/glm/first_level/first_level.py Show resolved Hide resolved

nilearn/interfaces/tests/test_bids.py Show resolved Hide resolved

add doc strings

3785882

ymzayek approved these changes Apr 11, 2023

View reviewed changes

nilearn/_utils/tests/test_data_gen.py Outdated Show resolved Hide resolved

Remi-Gau and others added 2 commits April 11, 2023 13:35

Update nilearn/_utils/tests/test_data_gen.py

ea368fc

Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>

typo

8155ec0

bthirion approved these changes Apr 11, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into slice_timing

de4a2a9

Remi-Gau merged commit 0b33453 into nilearn:main Apr 12, 2023
27 checks passed

Remi-Gau deleted the slice_timing branch April 12, 2023 09:38

Remi-Gau mentioned this pull request Apr 13, 2023

add a logger to "first_level_from_bids" #3686

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] properly infer slice_time_ref from BIDS derivatives #3605

[FIX] properly infer slice_time_ref from BIDS derivatives #3605

Remi-Gau commented Mar 18, 2023 •

edited

github-actions bot commented Mar 18, 2023 •

edited by Remi-Gau

codecov bot commented Mar 18, 2023 •

edited

Remi-Gau Mar 29, 2023

Remi-Gau commented Mar 31, 2023

Remi-Gau commented Mar 31, 2023 •

edited

bthirion left a comment

ymzayek left a comment

bthirion left a comment

ymzayek commented Apr 12, 2023

Remi-Gau commented Apr 12, 2023

[FIX] properly infer slice_time_ref from BIDS derivatives #3605

[FIX] properly infer slice_time_ref from BIDS derivatives #3605

Conversation

Remi-Gau commented Mar 18, 2023 • edited

github-actions bot commented Mar 18, 2023 • edited by Remi-Gau

codecov bot commented Mar 18, 2023 • edited

Codecov Report

Remi-Gau Mar 29, 2023

Choose a reason for hiding this comment

Remi-Gau commented Mar 31, 2023

Remi-Gau commented Mar 31, 2023 • edited

bthirion left a comment

Choose a reason for hiding this comment

ymzayek left a comment

Choose a reason for hiding this comment

bthirion left a comment

Choose a reason for hiding this comment

ymzayek commented Apr 12, 2023

Remi-Gau commented Apr 12, 2023

Remi-Gau commented Mar 18, 2023 •

edited

github-actions bot commented Mar 18, 2023 •

edited by Remi-Gau

codecov bot commented Mar 18, 2023 •

edited

Remi-Gau commented Mar 31, 2023 •

edited