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

Changes from HCP #1

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Changes from HCP #1

wants to merge 6 commits into from

Conversation

beOn
Copy link

@beOn beOn commented Feb 26, 2014

Hi There,

The Human Connectome Project has been using a modified version of gradunwarp for some time now.

I've been using some of their code, and the multiple versions of gradunwarp have given rise to some confusion and installation difficulties. So I asked them if they'd be alright with me submitting a pull request including a backwards compatible version of the changes, and got the go ahead. So here's that.

Please don't hesitate to get in touch about the changes. I did not personally write non_linear_unwarp_siemens, so don't have much to say about the decisions it makes, but can find the person who did if you have questions for them. This method only gets called if you provide the --hcp flag to the command line, so none of the current users should notice a change.

Ben

beOn added 6 commits February 26, 2014 10:09
added in the --hcp argument to avoid overloading filename.

It's possible that the two non_linear_unwarp methods could be merged in the future, but I didn't write either, and don't feel like fiddling with them. So here.
So, in the original hcp modifications, there was this m_rcs2lai_nohalf variable... but the line that applied halfvox to m_rcs2lai was also commented out. Now the halfvox stuff only happens when use_hcp_siemens is false, and there never was any difference between m_rcs2lai and m_rcs2lai_nohalf.

So I renamed all _nohalf to remove the suffix, but didn't notice that the two were being redundantly passed into use_hcp_siemens.
@beOn
Copy link
Author

beOn commented May 9, 2014

Bump.

I dropped you a message on LinkedIn... unless there's another Krish Subramaniam writing neuroimaging software. When you have time, please drop me a line if this repo is no longer actively maintained.

@ksubramz
Copy link
Owner

ksubramz commented May 9, 2014

Hi Ben

Yes, this repo is no longer actively maintained because I have left MGH and
I can't maintain it even if I want to because I don't have the data. I
wonder what's the best way to go about it right now. I will update the
github documentation to say that the repo is no longer maintained.

Krish

On Fri, May 9, 2014 at 3:47 PM, Ben Acland [email protected] wrote:

Bump.

I dropped you a message on LinkedIn... unless there's another Krish
Subramaniam writing neuroimaging software. When you have time, please drop
me a line if this repo is no longer actively maintained.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1#issuecomment-42706219
.

@beOn
Copy link
Author

beOn commented May 9, 2014

Thanks for the prompt reply!

I'm circling back with the HCP dev team to find out what they're open to. Right now, they bundle a modified version of gradunwarp with their code, but this can be a little confusing.

It's a little tough to approve a pull request on code you can't test, I understand. We might be able to look into getting some data to you, if you like. I don't know if that'll work out, but we can try.

The HCP team has been using the modified version for a while, and I did my best to ensure backwards compatibility. If you don't have time, but want more testing done before approving the pull request, let me know and I'll see if we can handle it on our end. Otherwise, it'd be very nice from our end to be able to tell people to install the latest master branch of your repo, and a pull would be much appreciated!

Ben

On May 9, 2014, at 2:52 PM, Krish Subramaniam [email protected] wrote:

Hi Ben

Yes, this repo is no longer actively maintained because I have left MGH and
I can't maintain it even if I want to because I don't have the data. I
wonder what's the best way to go about it right now. I will update the
github documentation to say that the repo is no longer maintained.

Krish

On Fri, May 9, 2014 at 3:47 PM, Ben Acland [email protected] wrote:

Bump.

I dropped you a message on LinkedIn... unless there's another Krish
Subramaniam writing neuroimaging software. When you have time, please drop
me a line if this repo is no longer actively maintained.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1#issuecomment-42706219
.


Reply to this email directly or view it on GitHub.

@ksubramz
Copy link
Owner

ksubramz commented May 9, 2014

Hi Ben

Is it hard for you guys to give the path to the cloned repo
(be0n::gradunwarp) to your users? I am afraid that my gradunwarp page is
basically a dead now so giving your users this path might lead them to
believe that this repo is actively maintained.

Regards

On Fri, May 9, 2014 at 4:06 PM, Ben Acland [email protected] wrote:

Thanks for the prompt reply!

I'm circling back with the HCP dev team to find out what they're open to.
Right now, they bundle a modified version of gradunwarp with their code,
but this can be a little confusing.

It's a little tough to approve a pull request on code you can't test, I
understand. We might be able to look into getting some data to you, if you
like. I don't know if that'll work out, but we can try.

The HCP team has been using the modified version for a while, and I did my
best to ensure backwards compatibility. If you don't have time, but want
more testing done before approving the pull request, let me know and I'll
see if we can handle it on our end. Otherwise, it'd be very nice from our
end to be able to tell people to install the latest master branch of your
repo, and a pull would be much appreciated!

Ben

On May 9, 2014, at 2:52 PM, Krish Subramaniam [email protected]
wrote:

Hi Ben

Yes, this repo is no longer actively maintained because I have left MGH
and
I can't maintain it even if I want to because I don't have the data. I
wonder what's the best way to go about it right now. I will update the
github documentation to say that the repo is no longer maintained.

Krish

On Fri, May 9, 2014 at 3:47 PM, Ben Acland [email protected]
wrote:

Bump.

I dropped you a message on LinkedIn... unless there's another Krish
Subramaniam writing neuroimaging software. When you have time, please
drop
me a line if this repo is no longer actively maintained.


Reply to this email directly or view it on GitHub<
https://github.com/ksubramz/gradunwarp/pull/1#issuecomment-42706219>
.


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1#issuecomment-42707924
.

@beOn
Copy link
Author

beOn commented May 9, 2014

Indeed, that might work. I was only working with their code for a little while, and made the pull request under my name because I knew how :).

Let me pass on what you've said to them, and check if they're open to maintaining their version in their own repo.

Thanks again, and thanks for the code :).

Ben

On May 9, 2014, at 3:16 PM, Krish Subramaniam [email protected] wrote:

Hi Ben

Is it hard for you guys to give the path to the cloned repo
(be0n::gradunwarp) to your users? I am afraid that my gradunwarp page is
basically a dead now so giving your users this path might lead them to
believe that this repo is actively maintained.

Regards

On Fri, May 9, 2014 at 4:06 PM, Ben Acland [email protected] wrote:

Thanks for the prompt reply!

I'm circling back with the HCP dev team to find out what they're open to.
Right now, they bundle a modified version of gradunwarp with their code,
but this can be a little confusing.

It's a little tough to approve a pull request on code you can't test, I
understand. We might be able to look into getting some data to you, if you
like. I don't know if that'll work out, but we can try.

The HCP team has been using the modified version for a while, and I did my
best to ensure backwards compatibility. If you don't have time, but want
more testing done before approving the pull request, let me know and I'll
see if we can handle it on our end. Otherwise, it'd be very nice from our
end to be able to tell people to install the latest master branch of your
repo, and a pull would be much appreciated!

Ben

On May 9, 2014, at 2:52 PM, Krish Subramaniam [email protected]
wrote:

Hi Ben

Yes, this repo is no longer actively maintained because I have left MGH
and
I can't maintain it even if I want to because I don't have the data. I
wonder what's the best way to go about it right now. I will update the
github documentation to say that the repo is no longer maintained.

Krish

On Fri, May 9, 2014 at 3:47 PM, Ben Acland [email protected]
wrote:

Bump.

I dropped you a message on LinkedIn... unless there's another Krish
Subramaniam writing neuroimaging software. When you have time, please
drop
me a line if this repo is no longer actively maintained.


Reply to this email directly or view it on GitHub<
https://github.com/ksubramz/gradunwarp/pull/1#issuecomment-42706219>
.


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1#issuecomment-42707924
.


Reply to this email directly or view it on GitHub.

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.

2 participants