Skip to content

[MAINT] Pin tedana version #2117

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 21 commits into from
May 13, 2020
Merged

[MAINT] Pin tedana version #2117

merged 21 commits into from
May 13, 2020

Conversation

@tsalo
Copy link
Collaborator

@tsalo tsalo commented May 5, 2020 •
edited
Loading

This PR pins the tedana version to 0.0.9a1, given several breaking changes since 0.0.8.

Changes proposed in this pull request:

  • Closes Update fMRIPrep requirements and T2SMap interface to 0.0.9 ME-ICA/tedana#540.
  • Pin tedana version to 0.0.9a1. There are a couple of breaking changes in the newest tedana release, including the following:
    • The t2smap workflow outputs nii.gz files instead of nii files.
    • The output files have been renamed as well, to better reflect BIDS conventions.
  • Additionally, tedana's added a slightly slower, more accurate method for T2*/S0 estimation using nonlinear estimation. I've changed the default to use this new "curvefit" method instead of the log-linear approximation that was originally used.

Documentation that should be reviewed

  • The init_bold_t2s_wf workflow __desc__ has been changed to reflect the improved T2*/S0 estimation method.
  • I also changed the title of the workflow description in the docs from T2* Driven Coregistration to T2*-driven echo combination. This fixes an oversight on my part from ENH: Remove t2s-coreg #2109.

tsalo added 11 commits April 1, 2020 19:43
@tsalo
Update tedana version.
cfab513
@tsalo
Remove full T2*/S0 maps from T2SMapOutputSpec.
e2bdeba
@tsalo
Change T2SMapOutputSpec filenames.
7505b34
@tsalo
Make curvefit default for T2*/S0 estimation.
9691541
@tsalo
Merge remote-tracking branch 'poldracklab/master' into maint/tedana-v…
191b240 
…ersion

# Conflicts:
#	setup.cfg
@tsalo
Drop TED directory and use default output directory (CWD).
5c3aa20
@tsalo
Merge remote-tracking branch 'poldracklab/master' into maint/tedana-v…
ce34324 
…ersion

# Conflicts:
#	setup.cfg
@tsalo
Fix t2smap output names.
d95928f
@tsalo
Merge remote-tracking branch 'poldracklab/master' into maint/tedana-v…
09fc5ea 
…ersion

# Conflicts:
#	setup.cfg
@tsalo
Update T2* workflow report to match new changes.
379f0bd
@tsalo
Merge remote-tracking branch 'poldracklab/master' into maint/tedana-v…
20f480c 
…ersion

# Conflicts:
#	setup.cfg
@auto-comment
Copy link

auto-comment bot commented May 5, 2020

Thank your for raising your pull request.

Some of the fMRIPRep maintainers will review your changes as soon as time permits.
I'm attaching below a Review Checklist for the reviewers, to check off during the
review.

PR Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

  • As the reviewer I confirm that there are no conflicts of interest for me to review this work.

Please check what applies in the following aspects of the PR:

Code documentation

  • New functions and modules are documented following the guidelines.
  • Existing code required amendments in documentation and has been updated.
  • Sufficient inline comments ensure readability by other contributors in the long term.

Documentation site

  • The modifications to fMRIPrep in this PR do not require extra documentation. This is a common situation when a bug fix that does not change the code base substantially is submitted.
  • This PR requires documentation. The corresponding documentation will be submitted as an additional PR in the future.
  • This PR requires extra documentation, and will be included within this PR. Please check the boxes that apply best:
    • An existing feature is substantially modified, changing the interface and/or logic of a module.
    • A new feature is being added, therefore full documentation of it will be included
    • This PR addresses an error in existing documentation.
  • Yes, all expected sections of documentation were generated by the CI build, and look as expected

Tests

  • The new functionalities in this PR are covered by the existing tests
  • New test batteries have been included with this PR

Data

  • This PR does not require any additional data to be uploaded to OSF.
  • This PR requires additional data for tests.

Dependencies: smriprep

  • This PR does not require any update on smriprep; or
  • smriprep has correctly been pinned.

Dependencies: niworkflows

  • This PR does not require any update on niworkflows; or
  • niworkflows has correctly been pinned.

Dependencies: sdcflows

  • This PR does not require any update on sdcflows; or
  • sdcflows has correctly been pinned.

Dependencies: Nipype

  • This PR does not require any update on nipype; or
  • nipype has correctly been pinned.

Dependencies: other

  • This PR does not require any update on other dependencies; or
  • other dependencies have correctly been pinned.

Reports generated within CI tests

  • Yes, I have checked the reports of ds005 and they are not modified or the changes are as expected
  • Yes, I have checked the reports of ds054 and they are not modified or the changes are as expected
  • Yes, I have checked the reports of ds010 and they are not modified or the changes are as expected

This was referenced May 5, 2020
Merged
Merged
@tsalo tsalo marked this pull request as ready for review May 10, 2020 20:52
mgxd
mgxd reviewed May 12, 2020
View reviewed changes
Copy link
Collaborator

@mgxd mgxd left a comment

Choose a reason for hiding this comment

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

👍, just a few comments.

setup.cfg Outdated
sdcflows @ git+https://github.com/nipreps/sdcflows.git@master
smriprep @ git+https://github.com/poldracklab/smriprep.git@master
tedana >= 0.0.5
tedana ~= 0.0.9a1
Copy link
Collaborator

@mgxd mgxd May 12, 2020

Choose a reason for hiding this comment

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

will this pick up 0.0.9? When pinning RCs in the past, we've had to do this manually, i.e.
>= 0.0.9a1, < 0.0.10

Copy link
Collaborator Author

@tsalo tsalo May 12, 2020

Choose a reason for hiding this comment

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

It was able to grab 0.0.9a1 in the tests, but I wasn't thinking about 0.0.9. Should I switch to >= 0.0.9a1, < 0.0.10 so it grabs 0.0.9 once it's released, or is it okay to pin to 0.0.9a1?

Copy link
Collaborator

@mgxd mgxd May 12, 2020

Choose a reason for hiding this comment

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

i would lean towards allowing grabbing a more stable version, but i'm not familiar with tedana's release cycle so whatever you think would be best

Copy link
Collaborator Author

@tsalo tsalo May 12, 2020

Choose a reason for hiding this comment

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

That should be fine. I'm happy to change it.

Copy link
Collaborator

@mgxd mgxd May 12, 2020

Choose a reason for hiding this comment

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

we can always increase/drop the maximum version, thanks!

@mgxd mgxd merged commit ed204e4 into nipreps:master May 13, 2020
@tsalo tsalo deleted the maint/tedana-version branch May 13, 2020 17:15
@oesteban oesteban mentioned this pull request Jun 2, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mgxd mgxd mgxd left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Update fMRIPrep requirements and T2SMap interface to 0.0.9

2 participants

@tsalo @mgxd