The new R package

[dorianps]

@muschellij2

Is there a reason to import only specific functions from ANTsR? Why not import the whole package and get done with it?

Dorian

[muschellij2]

That’s the preferred way and it tends to limit the overhead. Also you get the benefit if functions are removed or deprecated or moved you will be notified but that is not the case if you load the whole package

On Mon, Nov 5, 2018 at 5:31 PM dorianps notifications@github.com wrote:

@muschellij2 https://github.com/muschellij2

Is there a reason to import only specific functions from ANTsR? Why not import the whole package and get done with it?

Dorian

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dorianps/LINDA/issues/15, or mute the thread https://github.com/notifications/unsubscribe-auth/ABBnrpaSJ-9X2-ti2DzXr74RjvMnzjl9ks5usLwogaJpZM4YPY2V .

-- John

[muschellij2]

So what version of ANTsR did you have LINDA working with?

[muschellij2]

I wonder if the changes here made the difference: https://github.com/ANTsX/ANTsR/blob/cc3bc124a7d6b39d1aa530587add70951c6ecce4/R/mrvnrfs.R

I tried mrvnrfs.predict_chunks from (LINDA) and get the error

Error in rflist[[rfct]] : subscript out of bounds

but get

Error in predict.randomForest(rfm, newdata = fm, type = predtype) : 
  number of variables in newdata does not match that in the training data

when using ANTsR::mrvnrfs.predict

[muschellij2]

Note - that's not true for the LINDA code. I'm running now with that and we may use that going forward.

[dorianps]

I managed to make it work with the local linda mrvnrfs. There was a bug from my bad coding, expecting rfm$rflist somewhere in the code. That worked because the Rdata file contained rfm$rflist, which was found in the parent environment outside the function, but the new R package fed directly rflist. If you try my current repository now, should be good for that error.

This said, I think we can still fix things to make it work with the mrvnrf from ANTsR.

But now that I started over the example, there is an error in abpN4, which did not appear before because the output folder already contained the skull stripped files.

> linda_predict(filename)
20:32 Creating folder: /home/dorian/test-LINDA/linda
20:32 Loading file:
20:32 Loading template...
20:32 Skull stripping... (long process)
20:32 Running iteration 1
Error in (function (img, intensityTruncation = c(0.025, 0.975, 256), mask = NA,  : 
  unused arguments (tem = new("antsImage", pixeltype = "float", dimension = 3, components = 1, pointer = <pointer: 0xc65da20>, isVector = FALSE), temmask = new("antsImage", pixeltype = "float", dimension = 3, components = 1, pointer = <pointer: 0xd727720>, isVector = FALSE))
Called from: do.call(abpN4, args = args)
Browse[1]>

Do you know what is going on there?

I am using the latest ANTsR/Core from today.

The old installations I have are from July 2017:

> sessionInfo()
R version 3.2.5 (2016-04-14)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: CentOS release 6.9 (Final)

locale:
 [1] LC_CTYPE=en_US.UTF-8       LC_NUMERIC=C               LC_TIME=en_US.UTF-8       
 [4] LC_COLLATE=en_US.UTF-8     LC_MONETARY=en_US.UTF-8    LC_MESSAGES=en_US.UTF-8   
 [7] LC_PAPER=en_US.UTF-8       LC_NAME=C                  LC_ADDRESS=C              
[10] LC_TELEPHONE=C             LC_MEASUREMENT=en_US.UTF-8 LC_IDENTIFICATION=C       

attached base packages:
[1] stats     graphics  grDevices datasets  utils     methods   base     

other attached packages:
[1] LINDA_0.3.0         randomForest_4.6-12 ANTsR_0.6           ANTsRCore_0.3.7.4  

loaded via a namespace (and not attached):
 [1] Rcpp_0.12.18        rsvd_0.9            lattice_0.20-33     psych_1.8.4        
 [5] grid_3.2.5          magic_1.5-8         nlme_3.1-126        magrittr_1.5       
 [9] Matrix_1.2-11       ITKR_0.4.12.1       tools_3.2.5         foreign_0.8-66     
[13] RcppEigen_0.3.3.4.0 abind_1.4-5         parallel_3.2.5      mnormt_1.5-5  

[dorianps]

Btw, the example stroke worked until the end and produced a decent segmentation, albeit not matching older runs in full. The problem above come up after deleting the output and starting over again.

[dorianps]

@muschellij2 Just switched to SyNCC for the third run and got a segmentation as expected.

This is the comparison with the current release run on the latest ANTsR: white - current release red - R package

And this is the comparison with latest release run on the old ANTsR (1 yr old): white - old release old ANTsR red - current release latest ANTsR

So there is some stuff that changes with ANTsR I think, needs to be investigated, but at least the R package is giving the same output as the release.

I must say that the segmentation I sent you as a target example is really too old, probably from an even older version of LINDA. One thing that changed a couple of times is the reflection axis when computing the asymmetry mask. This example stroke is from a dataset that had issues with finding which side is the x-axis. When run with the wrong reflection axis, LINDA still worked, but produced a slightly smaller segmentation. This all without accounting from other changes in ANTsR... As you said, LINDA needs a lot of checks.

All said, we are at a good point now. I will search for other older examples to make comparisons. Thanks for taking care of transforming LINDA.

[muschellij2]

Ah - that's why I passed typeofTransform through but forgot to pass in the different SyNCC for prediction 3. Can we submit this to neuroconductor: https://neuroconductor.org/submit-package ?

[dorianps]

If you want to submit there the current version it's ok. But I think it's a bit premature, there are few more fixes and checks that are needed (display runtime, switch to cat, remove mrvnrfs 2-3-4 outputs, etc).

[dorianps]

Btw, I switched the template->ch2 to SyNCC as well. I am thinking we should just skip that step and tell the user to get the full release if MNI output is needed. This way we can just put transformations in the zip file and the release is ready.

[muschellij2]

You should not use cat, you should use message. Better output (coloring and differentiation) and also suppressMessages can be used to suppress. Can't really do that with cat John

On Tue, Nov 6, 2018 at 11:26 PM dorianps notifications@github.com wrote:

Btw, I switched the template->ch2 to SyNCC as well. I am thinking we should just skip that step and tell the user to get the full release if MNI output is needed. This way we can just put transformations in the zip file and the release is ready.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dorianps/LINDA/issues/15#issuecomment-436500852, or mute the thread https://github.com/notifications/unsubscribe-auth/ABBnrphA3ieSaYOuGBqj0YtaXGVo2Mu1ks5usmDqgaJpZM4YPY2V .

[muschellij2]

All outputs can be simply not written, but caching will not work then. But we could also have a cleanup = TRUE argument so that these files are deleted. If we don't care about the outputs, then we could just write to a temporary directory and not sub-dir of the directory of the T1.

John

On Tue, Nov 6, 2018 at 11:28 PM John Muschelli muschellij2@gmail.com wrote:

You should not use cat, you should use message. Better output (coloring and differentiation) and also suppressMessages can be used to suppress. Can't really do that with cat John

On Tue, Nov 6, 2018 at 11:26 PM dorianps notifications@github.com wrote:

Btw, I switched the template->ch2 to SyNCC as well. I am thinking we should just skip that step and tell the user to get the full release if MNI output is needed. This way we can just put transformations in the zip file and the release is ready.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dorianps/LINDA/issues/15#issuecomment-436500852, or mute the thread https://github.com/notifications/unsubscribe-auth/ABBnrphA3ieSaYOuGBqj0YtaXGVo2Mu1ks5usmDqgaJpZM4YPY2V .

[dorianps]

Here is why I dislike message:

1. Makes it very difficult to distinguish regular output from errors/warnings.
2. You can use capture.output to get and save all the prints or cats on a file (using this in LESYMAP). No way to capture messages.

Don't understand your point on outputs. I was just referring to the missing pennTemplate->ch2 transformations, which require another long registration. We should add an argument to eleminate that step if the user doesn't care about MNI (no need to wait 1+ more hours).

An overwrite argument is needed to start everything over in case.

What does the cache concept do?

[dorianps]

I think this might be coming from mrvrnfs, better be fixed in ANTsR, or eleminated with capture.output.

[muschellij2]

if cache = FALSE, then all things will be overwritten.

capture.output doesn't always capture all output (see system stdout and stderr). Also, harder to suppress in things like RMarkdown and output formats. Message or print is also recommended by RCore, see comments in: https://stackoverflow.com/questions/36699272/why-is-message-a-better-choice-than-print-in-r-for-writing-a-package/36700294

• those people John

On Tue, Nov 6, 2018 at 11:42 PM dorianps notifications@github.com wrote:

I think this might be coming from mrvrnfs, better be fixed in ANTsR, or eleminated with capture.output.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dorianps/LINDA/issues/15#issuecomment-436502912, or mute the thread https://github.com/notifications/unsubscribe-auth/ABBnrr8DdcMxpTqSGwK_du-BbeV6fFD5ks5usmShgaJpZM4YPY2V .

[dorianps]

Boh, not sure. Display method is easy to change anyway, thanks to your approach.

But lastly, with all the idiosyncrasy deriving from ANTsR versions, I think a sessionInfo output should always be saved with the results to increase the transparency and reproducibility efforts. I am struggling to understand from which version of which software each LINDA segmentation is coming from.

[muschellij2]

You should use sessioninfo::session_info() much more output there - especially when GitHub installed (SHA is kept). I do not recommend using sessionInfo().

Could also do sessioninfo::session_info("LINDA") for LINDA-specific packages.

John

On Tue, Nov 6, 2018 at 11:54 PM dorianps notifications@github.com wrote:

Boh, not sure. Display method is easy to change anyway, thanks to your approach.

But lastly, with all the idiosyncrasy deriving from ANTsR versions, I think a sessionInfo output should always be saved with the results to increase the transparency and reproducibility efforts. I am struggling to understand from which version of which software each LINDA segmentation is coming from.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dorianps/LINDA/issues/15#issuecomment-436504634, or mute the thread https://github.com/notifications/unsubscribe-auth/ABBnrlOqfqCSMZ0XQz1UFDdZY05_rC68ks5usmedgaJpZM4YPY2V .

[dorianps]

Yes that session command would be better also for me, but I think it needs another package, and, if that is the case, it's not worth to pretend it from the users.

Talk tomorrow.

On Tue, Nov 6, 2018, 11:59 PM John Muschelli <notifications@github.com wrote:

You should use sessioninfo::session_info() much more output there - especially when GitHub installed (SHA is kept). I do not recommend using sessionInfo().

Could also do sessioninfo::session_info("LINDA") for LINDA-specific packages.

John

On Tue, Nov 6, 2018 at 11:54 PM dorianps notifications@github.com wrote:

Boh, not sure. Display method is easy to change anyway, thanks to your approach.

But lastly, with all the idiosyncrasy deriving from ANTsR versions, I think a sessionInfo output should always be saved with the results to increase the transparency and reproducibility efforts. I am struggling to understand from which version of which software each LINDA segmentation is coming from.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dorianps/LINDA/issues/15#issuecomment-436504634, or mute the thread < https://github.com/notifications/unsubscribe-auth/ABBnrlOqfqCSMZ0XQz1UFDdZY05_rC68ks5usmedgaJpZM4YPY2V

.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/dorianps/LINDA/issues/15#issuecomment-436505208, or mute the thread https://github.com/notifications/unsubscribe-auth/AIqafY1KuqNjicQC98mY7wpvkwHopuDmks5usmidgaJpZM4YPY2V .

[muschellij2]

I agree for the most part but most users will have to have devtools to install LINDA which already will have this installed

On Wed, Nov 7, 2018 at 12:08 AM dorianps notifications@github.com wrote:

Yes that session command would be better also for me, but I think it needs another package, and, if that is the case, it's not worth to pretend it from the users.

Talk tomorrow.

On Tue, Nov 6, 2018, 11:59 PM John Muschelli <notifications@github.com wrote:

You should use sessioninfo::session_info() much more output there - especially when GitHub installed (SHA is kept). I do not recommend using sessionInfo().

Could also do sessioninfo::session_info("LINDA") for LINDA-specific packages.

John

On Tue, Nov 6, 2018 at 11:54 PM dorianps notifications@github.com wrote:

Boh, not sure. Display method is easy to change anyway, thanks to your approach.

But lastly, with all the idiosyncrasy deriving from ANTsR versions, I think a sessionInfo output should always be saved with the results to increase the transparency and reproducibility efforts. I am struggling to understand from which version of which software each LINDA segmentation is coming from.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dorianps/LINDA/issues/15#issuecomment-436504634, or mute the thread <

https://github.com/notifications/unsubscribe-auth/ABBnrlOqfqCSMZ0XQz1UFDdZY05_rC68ks5usmedgaJpZM4YPY2V

.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/dorianps/LINDA/issues/15#issuecomment-436505208, or mute the thread < https://github.com/notifications/unsubscribe-auth/AIqafY1KuqNjicQC98mY7wpvkwHopuDmks5usmidgaJpZM4YPY2V

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dorianps/LINDA/issues/15#issuecomment-436506446, or mute the thread https://github.com/notifications/unsubscribe-auth/ABBnrp-dJ35QDiXI3QiuUhQc3s49YRX6ks5usmqwgaJpZM4YPY2V .

-- John

[dorianps]

Solution: If devtools present, devtools::sessioninfo() else sessionInfo

[muschellij2]

devtools::session_info("LINDA") John

On Wed, Nov 7, 2018 at 9:17 AM dorianps notifications@github.com wrote:

Solution: If devtools present, devtools::sessioninfo() else sessionInfo

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dorianps/LINDA/issues/15#issuecomment-436636932, or mute the thread https://github.com/notifications/unsubscribe-auth/ABBnrlrAihgV06L6C8cv4mTAntXlMsbfks5usut4gaJpZM4YPY2V .

[dorianps]

Another to do:

• Pop up file selection window when no file is specified (compatibility with previous behavior).

[muschellij2]

Probably close this issue and make another?

[dorianps]

Just using this thread to keep track of all things still needing attention.

Another thing I remembered. The argument verbose was used before both to display information and to set ANTsR calls with verbosity. Is this still the case? They are two different concepts of verbosity.

[muschellij2]

You can set verbose = TRUE to show information, but verbose = 2 to show additional debugging information. John

On Wed, Nov 7, 2018 at 5:27 PM dorianps notifications@github.com wrote:

Just using this thread to keep track of all things still needing attention.

Another thing to I remembered. The argument verbose was used before both to display information and to set ANTsR calls with verbosity. Is this still the case? They are two different concepts of verbosity.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dorianps/LINDA/issues/15#issuecomment-436800934, or mute the thread https://github.com/notifications/unsubscribe-auth/ABBnrrANyJi12mpp2E2P797qkX1uncKzks5us15JgaJpZM4YPY2V .

[dorianps]

Applied a number of changes on display, output files, etc. Should be closer to final. Remains to run a couple cases to compare with the old version.

[dorianps]

@muschellij2 Ran a couple of old cases from last year and results are matching relatively well. Made a number of other changes, now at version 0.4.6, so it's all ready to go. You can give it a try if you like.

One thing I wanted to ask you is about dependencies. If we wanted to make it compatible for older ANTsR versions, what problems do you see? Looks like all functions we currently use have been there in older ANTsR (except is.antsImage maybe).

[dorianps]

Another to do: Add onLoad check for new version like LESYMAP.

[muschellij2]

So have you read the writing are extensions manual? I’ve never seen anyone check for updates on loading.

I think with ANTsR then you make the requirement and then going forward you aim for a reverse compatibility because it’s really hard to do this without being in the package management system in the past Unless you have a bunch of if statements

On Fri, Nov 9, 2018 at 12:26 AM dorianps notifications@github.com wrote:

Another to do: Add onLoad check for new version like LESYMAP.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dorianps/LINDA/issues/15#issuecomment-437254248, or mute the thread https://github.com/notifications/unsubscribe-auth/ABBnrhgjFSPDaB8NZGXVLuQbkbdwWBW7ks5utRH-gaJpZM4YPY2V .

-- John

[dorianps]

@muschellij2 I didn't understand your answer. If I take out all @importFrom and add @import ANTsR, do you foresee a reason why it would not work?

Getting the latest ANTsR is easy for us but users may be in the middle of a study and want to update ANTsR.

[muschellij2]

That should work fine. It’s just probably not the best practice in general but it’s not a problem here

On Fri, Nov 9, 2018 at 11:42 AM dorianps notifications@github.com wrote:

@muschellij2 https://github.com/muschellij2 I didn't understand your answer. If I take out all @importFrom and add @import ANTsR, do you foresee a reason why it would not work?

Getting the latest ANTsR is easy for us but users may be in the middle of a study and want to update ANTsR.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dorianps/LINDA/issues/15#issuecomment-437418919, or mute the thread https://github.com/notifications/unsubscribe-auth/ABBnrsXfdsQUw_0M8lrwdsl8ifhOw_fRks5utbCEgaJpZM4YPY2V .

-- John

[dorianps]

Enabled the package to run in older ANTsR, tested on a couple of cases using a 1yr old ANTsR, all works fine.

Finished everything and submitted to Neuroconductor. Thanks for the help @muschellij2.

Closing this issue.