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

allowing to read the new nifti TR pixdim field #132

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mezera
Copy link
Member

@mezera mezera commented Apr 15, 2015

allowing to read the new nifti convention that include the tr in the last pixdim field

@lmperry
Copy link
Member

lmperry commented Apr 16, 2015

@JWinawer, @arokem, can you comment on whether or not you think this change will adversely affect "normal" usage?

@arokem
Copy link
Member

arokem commented Apr 16, 2015

I might be wrong, but I think that the danger lies mainly in places in
which previously there was an assumption that the variable pixdim does
not have the TR.

For example, lines of code like the following:

https://github.com/vistalab/vistasoft/blob/master/mrDiffusion/GUI/dtiFiberUI.m#L735

Might have to be rewritten to refer explicitly to pixdim(1:3), but I have
not run any of the tests, so I can't point to any particular failure.

Is niftiNi2Vista.m on the main path for all nifti reads? Seems so, from a
quick look.

On Thu, Apr 16, 2015 at 11:26 AM, Michael Perry notifications@github.com
wrote:

@JWinawer https://github.com/JWinawer, @arokem
https://github.com/arokem, can you confirm that this change will not
adversely affect "normal" usage?


Reply to this email directly or view it on GitHub
#132 (comment).

@JWinawer
Copy link
Member

i will try to check. one question: did you (or anyone) try running the
validation code? (mrvTest)

that would be a good start.

On Thu, Apr 16, 2015 at 2:40 PM, Ariel Rokem notifications@github.com
wrote:

I might be wrong, but I think that the danger lies mainly in places in
which previously there was an assumption that the variable pixdim does
not have the TR.

For example, lines of code like the following:

https://github.com/vistalab/vistasoft/blob/master/mrDiffusion/GUI/dtiFiberUI.m#L735

Might have to be rewritten to refer explicitly to pixdim(1:3), but I have
not run any of the tests, so I can't point to any particular failure.

Is niftiNi2Vista.m on the main path for all nifti reads? Seems so, from a
quick look.

On Thu, Apr 16, 2015 at 11:26 AM, Michael Perry notifications@github.com
wrote:

@JWinawer https://github.com/JWinawer, @arokem
https://github.com/arokem, can you confirm that this change will not
adversely affect "normal" usage?


Reply to this email directly or view it on GitHub
#132 (comment).


Reply to this email directly or view it on GitHub
#132 (comment).

Jonathan Winawer
Assistant Professor of Psychology and Neural Science

New York University
6 Washington Place
New York, NY, 10003
(212) 998-7922 (phone)
(212) 995-4018 (fax)
jonathan.winawer@nyu.edu
http://psych.nyu.edu/winawer/

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.

4 participants