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: Find B0FieldIdentifiers when one image contributes to multiple #298

Merged
merged 11 commits into from
Dec 6, 2022

Conversation

bpinsard
Copy link
Contributor

Attempt at fixing/hacking #266

sdcflows/fieldmaps.py Outdated Show resolved Hide resolved
sdcflows/fieldmaps.py Outdated Show resolved Hide resolved
sdcflows/fieldmaps.py Outdated Show resolved Hide resolved
@bpinsard bpinsard force-pushed the fix/pepolar_b0fieldid_list branch 2 times, most recently from 8100cc0 to b37d8fe Compare December 3, 2022 02:19
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
@bpinsard bpinsard force-pushed the fix/pepolar_b0fieldid_list branch from b37d8fe to 56432af Compare December 3, 2022 02:22
sdcflows/fieldmaps.py Outdated Show resolved Hide resolved
bpinsard and others added 3 commits December 2, 2022 22:11
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
sdcflows/utils/wrangler.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Base: 85.38% // Head: 90.13% // Increases project coverage by +4.74% 🎉

Coverage data is based on head (68585e3) compared to base (83b064e).
Patch coverage: 90.47% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #298      +/-   ##
==========================================
+ Coverage   85.38%   90.13%   +4.74%     
==========================================
  Files          24       25       +1     
  Lines        1930     2372     +442     
  Branches      221      381     +160     
==========================================
+ Hits         1648     2138     +490     
+ Misses        246      189      -57     
- Partials       36       45       +9     
Flag Coverage Δ
travis 83.87% <90.00%> (-1.52%) ⬇️
unittests 90.13% <86.66%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
sdcflows/fieldmaps.py 98.25% <81.81%> (-1.16%) ⬇️
sdcflows/utils/wrangler.py 98.17% <100.00%> (+1.08%) ⬆️
sdcflows/utils/tools.py 90.81% <0.00%> (-9.19%) ⬇️
sdcflows/workflows/apply/correction.py 100.00% <0.00%> (ø)
sdcflows/conftest.py 0.00% <0.00%> (ø)
sdcflows/workflows/apply/registration.py 92.59% <0.00%> (+0.59%) ⬆️
sdcflows/workflows/outputs.py 92.72% <0.00%> (+0.79%) ⬆️
sdcflows/interfaces/reportlets.py 89.83% <0.00%> (+1.69%) ⬆️
sdcflows/transform.py 93.03% <0.00%> (+3.20%) ⬆️
... and 6 more

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@effigies effigies changed the title fix/hacks for pepolar scheme with B0FieldIdentifier as lists ENH: Find B0FieldIdentifiers when one image contributes to multiple Dec 5, 2022
@effigies
Copy link
Member

effigies commented Dec 5, 2022

@bpinsard Spent a bit of time on this logic today, and fixed the tests. Please check it on your use case.

@effigies
Copy link
Member

effigies commented Dec 6, 2022

Just a bump. If this is good to go, it would be nice to get in for a release.

@bpinsard
Copy link
Contributor Author

bpinsard commented Dec 6, 2022

Great. will test it asap.

@bpinsard
Copy link
Contributor Author

bpinsard commented Dec 6, 2022

B0FieldIdentifier=f'"{b0_id}"' works but repr(b0_id) gives single quote that are not matched, so if we want to use an existing function json.dumps is maybe a better choice?

@effigies
Copy link
Member

effigies commented Dec 6, 2022

Oh, are we doing json.dumps somewhere in pybids to store B0FieldIdentifier? I didn't see it. What does layout.get_B0FieldIdentifiers() look like on your data?

Will push f'"{b0_id}"' for now, though.

@bpinsard
Copy link
Contributor Author

bpinsard commented Dec 6, 2022

layout.get_B0FieldIdentifiers() will return string and tuples

In [10]: layout.get_B0FieldIdentifiers()
Out[10]: 
['sub_emotion_ses_002_task_emotion_run_01_part_mag_bold',
 'sub_emotion_ses_001_task_emotion_run_02',
 ('sub_emotion_ses_002_task_emotion_run_01_part_mag_bold',
  'sub_emotion_ses_002_task_emotion_run_02_part_mag_bold',
  'sub_emotion_ses_002_task_emotion_run_03_part_mag_bold'),
 'sub_emotion_ses_001_task_emotion_run_03_part_mag_bold',
 'sub_emotion_ses_002_task_emotion_run_04',
 'sub_emotion_ses_002_task_emotion_run_02_part_mag_bold',
 'sub_emotion_ses_002_task_emotion_run_03_part_mag_bold',
 'sub_emotion_ses_001_task_emotion_run_01_part_mag_bold',
 'sub_emotion_ses_001_task_emotion_run_04']

But the regexp matching is likely run on the json serialized string in pybids if I remember well (digged into code and sqlite database a while ago).

@effigies
Copy link
Member

effigies commented Dec 6, 2022

Okay. This should be working. @bpinsard Would you mind giving it one more try while we're waiting on the CI?

@bpinsard
Copy link
Contributor Author

bpinsard commented Dec 6, 2022

That works! 🎉

In [7]: sdcflows.utils.wrangler.find_estimators(layout=layout, subject='emotion')
Out[7]: 
[FieldmapEstimation(sources=<2 files>, method=<EstimatorType.PEPOLAR: 2>, bids_id='sub_emotion_ses_001_task_emotion_run_02'),
 FieldmapEstimation(sources=<2 files>, method=<EstimatorType.PEPOLAR: 2>, bids_id='sub_emotion_ses_001_task_emotion_run_01_part_mag_bold'),
 FieldmapEstimation(sources=<2 files>, method=<EstimatorType.PEPOLAR: 2>, bids_id='sub_emotion_ses_001_task_emotion_run_04'),
 FieldmapEstimation(sources=<2 files>, method=<EstimatorType.PEPOLAR: 2>, bids_id='sub_emotion_ses_001_task_emotion_run_03_part_mag_bold'),
 FieldmapEstimation(sources=<2 files>, method=<EstimatorType.PEPOLAR: 2>, bids_id='sub_emotion_ses_002_task_emotion_run_04'),
 FieldmapEstimation(sources=<2 files>, method=<EstimatorType.PEPOLAR: 2>, bids_id='sub_emotion_ses_002_task_emotion_run_03_part_mag_bold'),
 FieldmapEstimation(sources=<2 files>, method=<EstimatorType.PEPOLAR: 2>, bids_id='sub_emotion_ses_002_task_emotion_run_01_part_mag_bold'),
 FieldmapEstimation(sources=<2 files>, method=<EstimatorType.PEPOLAR: 2>, bids_id='sub_emotion_ses_002_task_emotion_run_02_part_mag_bold')]

@effigies effigies merged commit 82e7593 into nipreps:master Dec 6, 2022
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.

3 participants