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.

Sign up for GitHub

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

Jump to bottom

The new R package #15

Closed
dorianps opened this issue Nov 5, 2018 · 32 comments
Closed

The new R package #15

dorianps opened this issue Nov 5, 2018 · 32 comments

Comments

@dorianps
Copy link
Owner

dorianps commented Nov 5, 2018

@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
Copy link
Contributor

muschellij2 commented Nov 5, 2018 via email

@muschellij2
Copy link
Contributor

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

@muschellij2
Copy link
Contributor

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
Copy link
Contributor

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

@dorianps
Copy link
Owner Author

dorianps commented Nov 6, 2018

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
Copy link
Owner Author

dorianps commented Nov 6, 2018 •
edited
Loading

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
Copy link
Owner Author

dorianps commented Nov 7, 2018 •
edited
Loading

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

image

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

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
image

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
Copy link
Contributor

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
Copy link
Owner Author

dorianps commented Nov 7, 2018

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
Copy link
Owner Author

dorianps commented Nov 7, 2018

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
Copy link
Contributor

muschellij2 commented Nov 7, 2018 via email

@muschellij2
Copy link
Contributor

muschellij2 commented Nov 7, 2018 via email

@dorianps
Copy link
Owner Author

dorianps commented Nov 7, 2018 •
edited
Loading

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
Copy link
Owner Author

dorianps commented Nov 7, 2018 •
edited
Loading

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

image

@muschellij2
Copy link
Contributor

muschellij2 commented Nov 7, 2018 via email

@dorianps
Copy link
Owner Author

dorianps commented Nov 7, 2018

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
Copy link
Contributor

muschellij2 commented Nov 7, 2018 via email

@dorianps
Copy link
Owner Author

dorianps commented Nov 7, 2018 via email

@muschellij2
Copy link
Contributor

muschellij2 commented Nov 7, 2018 via email

@dorianps
Copy link
Owner Author

dorianps commented Nov 7, 2018

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

@muschellij2
Copy link
Contributor

muschellij2 commented Nov 7, 2018 via email

@dorianps
Copy link
Owner Author

dorianps commented Nov 7, 2018

Another to do:

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

@muschellij2
Copy link
Contributor

Probably close this issue and make another?

@dorianps
Copy link
Owner Author

dorianps commented Nov 7, 2018 •
edited
Loading

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
Copy link
Contributor

muschellij2 commented Nov 7, 2018 via email

@dorianps
Copy link
Owner Author

dorianps commented Nov 8, 2018

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
Copy link
Owner Author

dorianps commented Nov 9, 2018

@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
Copy link
Owner Author

dorianps commented Nov 9, 2018

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

@muschellij2
Copy link
Contributor

muschellij2 commented Nov 9, 2018 via email

@dorianps
Copy link
Owner Author

dorianps commented Nov 9, 2018

@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
Copy link
Contributor

muschellij2 commented Nov 9, 2018 via email

@dorianps
Copy link
Owner Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@muschellij2 @dorianps