search

vistalab / vistasoft

VISTASOFT is the main software repository of the VISTA lab at Stanford University.
http://vistalab.stanford.edu
148 stars 141 forks source link

Adding 32bit support for myCinterp3 #336

Open soichih opened 4 years ago

soichih commented 4 years ago

This PR updates myCinterp3 so that it can handle both 32bit and 64bit fibers coordinates.

We've also corrected the incorrect/inconsistent calculation of bounding box in dtiRoiNiftiFromMat.m.

wandell commented 4 years ago

Can you provide some tests we can include as part of the software so that we can validate the code and use the tests in the future to make sure further changes do not introduce problems? This principle - of additional testing and continuous integration - is something we are striving to achieve.

garikoitz commented 4 years ago

Hi! I just checked this PR and it is still failing downstream, I think that this change should be made backwards compatible. The problem is that the last change was already pushed to master, and now the previous version of this PR makes it fail. Can you roll it back or make your changes optional so that it does not fail downstream? Thanks! (to be clear, I am not saying there are problems in this specific PR, but I see all part of the same updating process you were doing, maybe the issue is only with the previous one... I can explain the tests I did in more detail)

soichih commented 4 years ago

@garikoitz I suggest making a branch from the commit that you know to work (maybe call it version 1.0?) and continue troubleshooting this issue on the master branch. To help you troubleshoot, can you send me the input data you are using, and how you are running vistasoft? From my point of view, without my PR vistasoft will crash due to out-of-memory. Whatever the problem is, I believe we can fix it.

garikoitz commented 4 years ago

I think we should separate the problems...

This PR that you merged: Reducing memory consumption for .tck tractograph loading and processing #320 makes the function dtiComputeDiffusionPropertiesAlongFG.m return only NaN values.

I understood that the new PR will fix that, but it did not. Can you make sure that the new PR fixes this problem before merging it?

(I think that I didn't do any changes, only suffered the consequences of the previously merged PR...)

Other checks I did:

I am only getting out of memory problems if I use very big whole brain tractograms, I usually do not have this problem, but I think that your changes are going to be very helpful, but right now it is breaking other code that we should fix.

Dataset: you can use any dataset, it is failing for us with Siemens, GE, multishell, single shell... it is not dataset related. I didn't dig into the new code to understand the problem further, just where it happens.

Thanks for looking at this! Gari

On Fri, Jul 17, 2020 at 3:58 PM Soichi Hayashi notifications@github.com wrote:

@garikoitz https://github.com/garikoitz I suggest making a branch from the commit that you know to work (maybe call it version 1.0?) and continue troubleshooting this issue on the master branch. To help you troubleshoot, can you send me the input data you are using, and how you are running vistasoft? From my point of view, without my PR vistasoft will crash due to out-of-memory. Whatever the problem is, I believe we can fix it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/vistalab/vistasoft/pull/336#issuecomment-660121813, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABCZAV4BAN3WPEN6UICFY6DR4BKIJANCNFSM4OBZR7EQ .

francopestilli commented 4 years ago

hi @garikoitz we want to help with this. I think we need more details on what is failing and why. Would it be possible to share data and a script that we can work on, please? We could make a test using the files that are failing for you.

wandell commented 4 years ago

Thanks for offering to help. Here is a suggestion as to what would be most helpful, I think. Set up a suite of system and unit tests. Share them. Then run them before doing a merge into the master branch. If you got all the way to CI, that would be even better.

Sent from my iPad

From: Franco Pestilli notifications@github.com Sent: Friday, July 17, 2020 7:48:14 AM To: vistalab/vistasoft vistasoft@noreply.github.com Cc: Brian A Wandell wandell@stanford.edu; Comment comment@noreply.github.com Subject: Re: [vistalab/vistasoft] Adding 32bit support for myCinterp3 (#336)

hi @garikoitzhttps://github.com/garikoitz would want to help. But perhaps this exchange needs more details. Would it be possible to share data and a script that we can work on, please? We could make a test using the files that are failing for you.

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/vistalab/vistasoft/pull/336#issuecomment-660148398, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAOAQWKO7YZI6JQBUL6SQYTR4BQC3ANCNFSM4OBZR7EQ.

garikoitz commented 4 years ago

Thanks Franco, I am using compiled code and Docker containers, do you want to have the containers? we will need to configure it all...

I would say, if you take any dataset (HCP) that you obtained any tck (arcuate) and read it into a fg and then launch the function AFQ_ComputeTractProperties. In my experience, using the code before that merge, it returns correct values, after that merge, it returns only nans. If you can't reproduce it, we can start trying to see what would be the best way to give you the code and configs, no problem with that!

On Fri, Jul 17, 2020 at 5:12 PM Brian Wandell notifications@github.com wrote:

Thanks for offering to help. Here is a suggestion as to what would be most helpful, I think. Set up a suite of system and unit tests. Share them. Then run them before doing a merge into the master branch. If you got all the way to CI, that would be even better.

Sent from my iPad

From: Franco Pestilli notifications@github.com Sent: Friday, July 17, 2020 7:48:14 AM To: vistalab/vistasoft vistasoft@noreply.github.com Cc: Brian A Wandell wandell@stanford.edu; Comment < comment@noreply.github.com> Subject: Re: [vistalab/vistasoft] Adding 32bit support for myCinterp3 (#336)

hi @garikoitzhttps://github.com/garikoitz would want to help. But perhaps this exchange needs more details. Would it be possible to share data and a script that we can work on, please? We could make a test using the files that are failing for you.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub< https://github.com/vistalab/vistasoft/pull/336#issuecomment-660148398>, or unsubscribe< https://github.com/notifications/unsubscribe-auth/AAOAQWKO7YZI6JQBUL6SQYTR4BQC3ANCNFSM4OBZR7EQ

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/vistalab/vistasoft/pull/336#issuecomment-660161538, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABCZAV7TOXETSRQ4Y37QIDTR4BS4JANCNFSM4OBZR7EQ .