Substantial overhead on math operations

[dorianps]

I was using the default ANTsR functions for some math operations, and turn out they are much slower than converting the image with as.array() and applying the function.

img <- antsImageRead( getANTsRData( "r16" ) )

system.time(for (i in 1:1000) sum(img))
# user  system elapsed 
# 2.594   0.000   2.614 
system.time(for (i in 1:1000) sum(as.array(img)))
# user  system elapsed 
# 0.687   0.000   0.705

This translates in 6s vs 60s difference in 130 real images. Tried both sum and range, on two different systems, with old and latest ANTsR, so I think it's happening on all math operations.

I thought we resolved this problem a couple of years ago. Any idea what is going on and how to resolve.

[muschellij2]

You need to run devtools::session_info() to see what versions of the packages you have. Otherwise, this is irrelevant.

[muschellij2]

Also, you can pass in a mask into the sum for antsImage:

            args = list(...)
            mask = args$mask
            args$mask = NULL
            x = mask_values(x, mask)
            # I think this makes sense but should ask Avants.
            # relevant for warnings for all/any in summary
            if (all(x %in% c(0, 1, NA, NaN))) {
              x = as.logical(x)
            }
            args$x = x
            args$na.rm = na.rm
            res = do.call(callGeneric, args = args)

The slow part is

            # I think this makes sense but should ask Avants.
            # relevant for warnings for all/any in summary
            if (all(x %in% c(0, 1, NA, NaN))) {
              x = as.logical(x)
            }

[dorianps]

> devtools::session_info()
Session info ------------------------------------------------------------------
 setting  value
 version  R version 3.5.0 (2018-04-23)
 system   x86_64, linux-gnu
 ui       X11
 language (EN)
 collate  en_US.UTF-8
 tz       America/New_York
 date     2018-08-09

Packages ----------------------------------------------------------------------
 package   * version   date       source
 abind       1.4-5     2016-07-21 cran (@1.4-5)
 ANTsR     * 0.4.4.1   2018-08-07 Github (stnava/ANTsR@6c00733)
 ANTsRCore * 0.5.6.1   2018-08-07 Github (stnava/ANTsRCore@7b01990)
 base      * 3.5.0     2018-05-13 local
 compiler    3.5.0     2018-05-13 local
 datasets  * 3.5.0     2018-05-13 local
 devtools    1.13.5    2018-02-18 CRAN (R 3.5.0)
 digest      0.6.15    2018-01-28 CRAN (R 3.5.0)
 foreign     0.8-70    2017-11-28 CRAN (R 3.5.0)
 graphics  * 3.5.0     2018-05-13 local
 grDevices * 3.5.0     2018-05-13 local
 grid        3.5.0     2018-05-13 local
 ITKR        0.4.14.0  2018-08-07 Github (stnava/ITKR@ff47594)
 lattice     0.20-35   2017-03-25 CRAN (R 3.5.0)
 magic       1.5-8     2018-01-26 cran (@1.5-8)
 magrittr    1.5       2014-11-22 cran (@1.5)
 Matrix      1.2-14    2018-04-13 CRAN (R 3.5.0)
 memoise     1.1.0     2017-04-21 CRAN (R 3.5.0)
 methods   * 3.5.0     2018-05-13 local
 mnormt      1.5-5     2016-10-15 cran (@1.5-5)
 nlme        3.1-137   2018-04-07 CRAN (R 3.5.0)
 parallel    3.5.0     2018-05-13 local
 psych       1.8.4     2018-05-06 cran (@1.8.4)
 Rcpp        0.12.18   2018-07-23 cran (@0.12.18)
 RcppEigen   0.3.3.4.0 2018-02-07 cran (@0.3.3.4)
 rsvd        0.9       2017-12-08 cran (@0.9)
 stats     * 3.5.0     2018-05-13 local
 tools       3.5.0     2018-05-13 local
 utils     * 3.5.0     2018-05-13 local
 withr       2.1.2     2018-03-15 CRAN (R 3.5.0)

[muschellij2]

Yeah this is due to the check for the warnings for all and any. We can take these out for speed, which may be a good idea.

> system.time(for (i in 1:1000) all(img>0))
   user  system elapsed 
  3.496   1.020   4.810 
> system.time(for (i in 1:1000) all(as.array(img>0)))
   user  system elapsed 
  1.420   0.376   1.754 
There were 50 or more warnings (use warnings() to see the first 50)

3x slowdown, but you don't get any warnings.

[dorianps]

Another idea is add Travis tests so we can detect immediately changes that impact performance in the future. At least on basic operations this would be very helpful.

[muschellij2]

The relevant comment/code is in https://github.com/ANTsX/ANTsRCore/blob/master/R/zzz_Summary.R#L54. So @stnava should we have warnings or not?

Can you make those tests in travis @dorianps ?

[dorianps]

To be honest I've never added proper tests to packages, and need to dig a little more. It would be a matter of using those lines above to fail the test if elpsed time passes 1s. May check onto this later, for now we need to remove unnecessary overhead I think.

[muschellij2]

OK - can you send a pull request?

John

On Thu, Aug 9, 2018 at 4:12 PM, dorianps notifications@github.com wrote:

To be honest I've never added proper tests to packages, and need to dig a little more. It would be a matter of using those lines above to fail the test if elpsed time passes 1s. May check onto this later, for now we need to remove unnecessary overhead I think.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ANTsX/ANTsR/issues/233#issuecomment-411882195, or mute the thread https://github.com/notifications/unsubscribe-auth/ABBnri8NkJNTT1hBGFt4qzGPP_1oHGonks5uPJe6gaJpZM4V2Tfi .

[dorianps]

Will look into making a test after the speedups are merged into master.

[stnava]

I agree that the performance should be prioritized for these common operations. The performance hit definitely adds up dealing with larger data sets.

However I think we should comment out the warning handling in case we can think of a way to have both options via session variable so the user can choose how to operate.

On Thu, Aug 9, 2018 at 4:17 PM John Muschelli notifications@github.com wrote:

OK - can you send a pull request?

John

On Thu, Aug 9, 2018 at 4:12 PM, dorianps notifications@github.com wrote:

To be honest I've never added proper tests to packages, and need to dig a little more. It would be a matter of using those lines above to fail the test if elpsed time passes 1s. May check onto this later, for now we need to remove unnecessary overhead I think.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ANTsX/ANTsR/issues/233#issuecomment-411882195, or mute the thread < https://github.com/notifications/unsubscribe-auth/ABBnri8NkJNTT1hBGFt4qzGPP_1oHGonks5uPJe6gaJpZM4V2Tfi

.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/ANTsX/ANTsR/issues/233#issuecomment-411883480, or mute the thread https://github.com/notifications/unsubscribe-auth/AATyfqMdx8nv7yZDxAQxQxV4HllL1j96ks5uPJjZgaJpZM4V2Tfi .

--

brian

[muschellij2]

@dorianps I'm saying make a PR for merging the speedups.

[dorianps]

I wouldn't want to mess things up in zzz as I know little on what is going on there. If you want me just to comment out those three lines https://github.com/ANTsX/ANTsRCore/blob/master/R/zzz_Summary.R#L54-L56, I can do that.

[dorianps]

Or, of course, I can try the suggested fix later/tomorrow and do some more tests to make sure the speed gain is there.

[stnava]

Dorian, we are all working on different parts of the codes will be helpful if you can just comment out the time-consuming part is test and verify that it’s producing speed up and make a pull request so we can easily merge.

On Thu, Aug 9, 2018 at 4:34 PM dorianps notifications@github.com wrote:

Or, of course, I can try the suggested fix later/tomorrow and do some more tests to make sure the speed gain is there.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/ANTsX/ANTsR/issues/233#issuecomment-411888146, or mute the thread https://github.com/notifications/unsubscribe-auth/AATyfvd24k3VlaHzWaBcOXmDHnrTrpE8ks5uPJzggaJpZM4V2Tfi .

--

brian

[dorianps]

Ok, will do.

[dorianps]

Partially fixed with PR https://github.com/ANTsX/ANTsRCore/pull/58. Closing.