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: Port bbregister T1-to-dwi registration from fmriprep #125

Merged
merged 9 commits into from
Dec 4, 2020

Conversation

slimnsour
Copy link
Contributor

Hello, this is to address #115 by adding bbregister to dmriprep. As you can see there is a little more work to be done because I wanted feedback on some things. Specifically:

  1. Is there a best place I should put this workflow in? Ex. dmriprep/workflows/dwi/base.py or dmriprep/workflows/dwi/registration.py?
  2. How should I import init_bbreg_wf in this case? I would have just imported init_bbreg_wf from fmriprep but fmriprep is not part of the packages installed in dmriprep. What is the suggested practice here?

Let me know of any feedback and I'll get to them.

@welcome
Copy link

welcome bot commented Oct 29, 2020

Thanks for opening this pull request! We have detected this is the first time for you to contribute to dMRIPrep. Please check out our contributing guidelines.
We invite you to list yourself as a dMRIPrep contributor, so if your name is not already mentioned, please modify the .zenodo.json file and insert your data last in the contributors list. Example:

{
   "name": "Contributor, New dMRIPrep",
   "affiliation": "Department of dMRI prep'ing, Open Science Made-Up University",
   "orcid": "<your id>",
   "type": "Researcher"
}

Of course, if you want to opt-out this time there is no problem at all with adding your name later. You will be always welcome to add it in the future whenever you feel it should be listed.

Copy link
Member

@oesteban oesteban left a comment

Choose a reason for hiding this comment

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

Looking great - I think we can import the init_bbreg_wf from fMRIPrep (I'll move it to smriprep or niworkflows, so that we do not depend on fmriprep). Please confirm.

dmriprep/cli/parser.py Outdated Show resolved Hide resolved
dmriprep/config/__init__.py Outdated Show resolved Hide resolved
dmriprep/workflows/base.py Outdated Show resolved Hide resolved
dmriprep/workflows/base.py Outdated Show resolved Hide resolved
dmriprep/workflows/dwi/registration.py Outdated Show resolved Hide resolved
@pep8speaks
Copy link

pep8speaks commented Dec 3, 2020

Hello @slimnsour, Thank you for updating!

Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally, pip install flake8 and then run flake8 dmriprep.

Comment last updated at 2020-12-04 17:26:32 UTC

Copy link
Member

@oesteban oesteban left a comment

Choose a reason for hiding this comment

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

I'm going to add three stupid changes to your comments. Thanks for your understanding.

dmriprep/workflows/base.py Outdated Show resolved Hide resolved
dmriprep/workflows/base.py Outdated Show resolved Hide resolved
dmriprep/workflows/base.py Outdated Show resolved Hide resolved
@oesteban
Copy link
Member

oesteban commented Dec 4, 2020

@slimnsour please add your info to

{
"affiliation": [
"Basque Center on Cognition, Brain and Language (BCBL)",
"Department of Psychology, Stanford University"
],
"name": "Lerma-Usabiaga, Garikoitz",
"orcid": "0000-0001-9800-4816"
},
{
"affiliation": "Department of Psychology, University of Texas at Austin, TX, USA",
"name": "Pisner, Derek",
"orcid": "0000-0002-1228-0201"
},

(it is ordered alphabetically by lastname)

@oesteban
Copy link
Member

oesteban commented Dec 4, 2020

Shoot, I've overwritten your changes - assumed I was changing history (and I shouldn't have).

Can you add the lines here as a comment? @slimnsour

@slimnsour
Copy link
Contributor Author

No worries, here it is.

{
   "affiliation": "The Centre for Addiction and Mental Health",
   "name": "Mansour, Salim",
   "orcid": "0000-0002-1092-1650"
},

@oesteban
Copy link
Member

oesteban commented Dec 4, 2020

Works locally. I'm going to merge. Thanks for this great work @slimnsour and sorry for my slow response all the time.

@oesteban oesteban merged commit 6e409a2 into nipreps:master Dec 4, 2020
oesteban added a commit to oesteban/niworkflows that referenced this pull request Dec 4, 2020
oesteban added a commit to oesteban/niworkflows that referenced this pull request Dec 4, 2020
oesteban added a commit to oesteban/niworkflows that referenced this pull request Dec 4, 2020
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.

3 participants