search

ANTsX / ANTsR

R interface to the ANTs biomedical image processing library
https://antsx.github.io/ANTsR
Apache License 2.0
127 stars 35 forks source link

move to r-only #393

Closed ncullen93 closed 8 months ago

ncullen93 commented 9 months ago

This PR - in tandem with https://github.com/ANTsX/ANTsRCore/pull/161 moves in all the R code to ANTsR and moves out all of the C++ code to ANTsRCore. Now, ANTsR is a pure R package that just depends (vitally) on ANTsRCore. From a dev perspective, this means ANTsR should never make any .Call calls - those are simply replaced to the wrapper for the corresponding .Call function in ANTsRCore and ANTsR will call ANTsRCore functions internally instead.

This is a big step towards getting on CRAN because we can now focus on normal R package issues here without worrying about the ANTs build failing or taking a ton of time. The build size for ANTsR is now <1mb.

Also, this change is almost certainly necessary to get on CRAN/bioconductor because currently ANTsR is just importing a ton of ANTsRCore functions into the namespace without wrapping them... so those functions would have an ANTsRCore:: prefix. All the user-facing functions need to be in the same namespace because people (like me) actually use the package prefix during development.

The major issue I have with this PR is that not all the tests passed.. in particular the mergeChannels test don't seem to be correct (EDIT: fixed - was a small typo), although all the functions run. I haven't confirmed that all the tests actually pass in the current version of ANTsR/ANTsRCore though.

ncullen93 commented 9 months ago

Update - fixed the mergeChannels issue.. was just a small typo in the R code thankfully. So beautiful to be able to test and re-install ANTsR in <10 seconds. Im confident the same number of tests pass in this antsrcore+antsr setup as with the current one.

ncullen93 commented 9 months ago

Update - I fixed all dependency check errors, so now R CMD Check actually runs all the way through. There are tons of other errors / warnings / notes but all addressable within a reasonable timeframe.

Happy to hear thoughts on whether this is a viable approach or if there's a better one.

ncullen93 commented 8 months ago

It's been a month.. any thoughts @stnava ? All it does is move all c++ code to antsrcore and make antsr an R-only package for easier maintanence and building.

I think we should merge this big change and then I will take a few weeks to clean up the code, make it pass R CMD CHECK, and try to get it on cran / bioconductor. If that doesn't work, you can just revert back. I see only upside here compared to the current situation.

stnava commented 8 months ago

that's fine with me - I wont have time to review properly in the near term

ncullen93 commented 8 months ago

I really appreciate it.. will update you on the status in a few weeks.