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] Add CE agent to output figure filename templates #482

Merged
merged 9 commits into from Mar 19, 2020

Conversation

jdkent
Copy link
Collaborator

@jdkent jdkent commented Mar 18, 2020

supercedes #479
addresses nipreps/fmriprep#2033

@pull-assistant
Copy link

pull-assistant bot commented Mar 18, 2020

Score: 0.95

Best reviewed: commit by commit


Optimal code review plan

     replace ce with ceagent

     test ceagent

     add ceagent as needed_entity for tests

     ensure one file per reportlet

     change dir to direction

     change expected size of output

     ensure titles have the correct keys

     add the required entities to figures.json

     ensure all the figures are copied over

Powered by Pull Assistant. Last update 4ae4176 ... eb1c4bd. Read the comment docs.

@@ -21,15 +21,15 @@
"pattern": "[_/\\\\]acq-([a-zA-Z0-9]+)"
},
{
"name": "ce",
"name": "ceagent",
Copy link
Member

@effigies effigies Mar 18, 2020

Choose a reason for hiding this comment

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

Does this have any API effect?

Copy link
Collaborator Author

@jdkent jdkent Mar 18, 2020

Choose a reason for hiding this comment

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

No, I do not believe so. This does not impact the recently added --bids-filter, and makes figures.json naming consistent with fmriprep.yml.

Copy link
Member

@effigies effigies Mar 18, 2020

Choose a reason for hiding this comment

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

Okay. @oesteban Is this consistent with your understanding as well?

Copy link
Member

@oesteban oesteban Mar 18, 2020

Choose a reason for hiding this comment

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

Yup, I also think this should not affect the API

@effigies
Copy link
Member

effigies commented Mar 18, 2020

Actually, since #479 isn't merged, let's just make this the main PR, and we'll merge into master after the next release. (Assuming that this is API neutral and safe to put in a bug-fix.)

@effigies effigies changed the title [FIX] port #479 (Fix ceagent) to maint/1.1.x [FIX] Add CE agent to output figure filename templates Mar 18, 2020
@codecov
Copy link

codecov bot commented Mar 19, 2020

Codecov Report

Merging #482 into maint/1.1.x will decrease coverage by 5.17%.
The diff coverage is 100.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           maint/1.1.x     #482      +/-   ##
===============================================
- Coverage        62.93%   57.76%   -5.18%     
===============================================
  Files               41       40       -1     
  Lines             5005     4991      -14     
  Branches           727      726       -1     
===============================================
- Hits              3150     2883     -267     
- Misses            1704     1980     +276     
+ Partials           151      128      -23     
Flag Coverage Δ
#documentation ?
#reportlettests ?
#travis 57.76% <100.00%> (-0.03%) ⬇️
#unittests ?
Impacted Files Coverage Δ
niworkflows/reports/core.py 72.09% <100.00%> (-0.48%) ⬇️
niworkflows/func/util.py 25.00% <0.00%> (-62.50%) ⬇️
niworkflows/anat/ants.py 12.15% <0.00%> (-57.46%) ⬇️
niworkflows/anat/freesurfer.py 39.13% <0.00%> (-52.18%) ⬇️
niworkflows/anat/skullstrip.py 30.00% <0.00%> (-50.00%) ⬇️
niworkflows/interfaces/itk.py 26.92% <0.00%> (-12.31%) ⬇️
niworkflows/interfaces/fixes.py 41.17% <0.00%> (-11.77%) ⬇️
niworkflows/interfaces/bids.py 80.15% <0.00%> (-10.32%) ⬇️
niworkflows/interfaces/ants.py 57.81% <0.00%> (-7.82%) ⬇️
niworkflows/interfaces/utils.py 37.76% <0.00%> (-4.99%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2bf5430...eb1c4bd. Read the comment docs.

@effigies
Copy link
Member

effigies commented Mar 19, 2020

I've tested with a synthetic dataset where I switched from run-* to ce-*, and fmriprep 20.0.4 + this PR works as expected.

Given @bbfrederick's comments, I'm going to go ahead and merge.

Thanks!

@effigies effigies merged commit 1e2b5f3 into nipreps:maint/1.1.x Mar 19, 2020
7 of 8 checks passed
@bbfrederick
Copy link

bbfrederick commented Mar 19, 2020

Works for me. I'll wait until the dust settles, sync up with the patched release version of niworkflows, and then put in a PR for the BIDS_NAME thing. AFAIK it doesn't actually cause any problems at the moment, but it is wrong and it might bite somebody later if they decide to use it, so it's not currently urgent.

effigies added a commit that referenced this pull request Mar 19, 2020
1.1.12 (March 19, 2020)

Bug-fix release in the 1.1.x series.

* FIX: Update naming patterns in figures.json (#483)
* FIX: Add CE agent to output figure filename templates (#482)
effigies added a commit to nipreps/fmriprep that referenced this pull request Mar 19, 2020
20.0.5 (March 19, 2020)

Bug-fix release in 20.0.x series.

With thanks to James Kent for the fix and Blaise Frederick for the report and testing.

* FIX: Add CE agent to output figure filename templates (nipreps/niworkflows#482)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants