Adding 32bit support for myCinterp3

[soichih]

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]

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]

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]

@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]

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: - old LiFE and Encode & latest LiFE and Encode, master vistasoft: returns NaNs - old LiFE and Encode & latest LiFE and Encode, old vistasoft (before #320): returns ok values 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 ***@***.***> 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 <#336 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABCZAV4BAN3WPEN6UICFY6DR4BKIJANCNFSM4OBZR7EQ> .

[francopestilli]

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]

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 <[email protected]> Sent: Friday, July 17, 2020 7:48:14 AM To: vistalab/vistasoft <[email protected]> Cc: Brian A Wandell <[email protected]>; Comment <[email protected]> Subject: Re: [vistalab/vistasoft] Adding 32bit support for myCinterp3 (#336) hi @garikoitz<https://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<#336 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAOAQWKO7YZI6JQBUL6SQYTR4BQC3ANCNFSM4OBZR7EQ>.

[garikoitz]

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 ***@***.***> 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 ***@***.***> Sent: Friday, July 17, 2020 7:48:14 AM To: vistalab/vistasoft ***@***.***> Cc: Brian A Wandell ***@***.***>; Comment < ***@***.***> Subject: Re: [vistalab/vistasoft] Adding 32bit support for myCinterp3 (#336) hi @garikoitz<https://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< #336 (comment)>, 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 <#336 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABCZAV7TOXETSRQ4Y37QIDTR4BS4JANCNFSM4OBZR7EQ> .