dodgr_paths pairwise performance

[dcooley]

Would you consider moving the for loop here in dodgr_paths() to C++?

In my fork I've made two test functions

R - dodgr_paths_pairwise C++ - rcpp_get_paths_pairwise

And they benchmark well

graph <- weight_streetnet (hampi)
from <- sample (graph$from_id, size = 100)
to <- sample (graph$to_id, size = 100)

microbenchmark::microbenchmark(

  r_loop = {
    dp <- dodgr_paths (graph, from = from, to = to, pairwise = TRUE)
  },
  c_loop = {
    dp2 <- dodgr:::dodgr_paths_pairwise(graph, from = from, to = to, pairwise = TRUE )
  },
  times = 25
)

# Unit: milliseconds
#   expr       min        lq      mean    median        uq       max neval
# r_loop 398.95610 406.17030 425.67352 413.67783 441.19254 499.53343    25
# c_loop  17.73244  18.58716  19.56402  19.15842  20.57666  22.12603    25

all.equal(dp, dp2)
# [1] TRUE

(This would also null-and-void #148 )

[mpadge]

Yeah, totally. The pairwise was admittedly a bit of an afterthought, so isn't quiet so sensibly integrated. As always, feel free to PR, since you've got the code on your fork anyway.

[dcooley]

will do!

[mpadge]

@dcooley Gentle nudge: Can you just PR your branch so we can close this, please? I don't think anything much has been done on paths code in the meantime, so there shouldn't be any merge conflicts. Thanks!

[dcooley]

Yes, but I need to re-work it, because I started changing other things as I was going along, and got in a bit of a mess, and then I stopped working on it for various reasons.

But I'll get on it today!

[dcooley]

ok slight problem; I can't build it on my Mac (M1 chip). Have you encountered this by any chance?

[mpadge]

Thanks Dave, and sorry to report I haven't even tried on Mac. I did nevertheless just add an extra workflow called "extra-R-CMD-check" which runs both r-release and r-devel on the GitHub MacOS runners which are M1 machines. You'll at least be able to see there via PR if that fails.

[dcooley]

Ok I've got working on my M1 system by following the steps here

1. Update MakeConf

/Library/Frameworks/R.framework/Resources/etc/Makeconf

2. Change FLIBS line

#FLIBS =  -L/opt/R/arm64/gfortran/lib/gcc/aarch64-apple-darwin20.2.0/11.0.0 -L/opt/R/arm64/gfortran/lib -lgfortran -lemutls_w -lm
# change to
FLIBS = -L/usr/local/gfortran/lib/gcc/aarch64-apple-darwin20.2.0/11.0.0 -L/usr/local/gfortran/lib -lgfortran -lm

3 Change FC line

#FC = /opt/R/arm64/bin/gfortran -mtune=native
## change to
FC = /usr/local/gfortran/bin/gfortran -mtune=native

I don't know the full impact of these changes, but I will now do some tests and make the PR...

[dcooley]

I'm getting hard-crashes runing a lot of the examples. e.g, this

graph <- weight_streetnet (hampi)

won't run and aborts RStudio...

I don't know if this is realated to the fortran changes I made, or something else is awry

[dcooley]

It looks like my messing around with Makeconf didn't actually fix it. I'll keep trying

[mpadge]

@dcooley Long time dormant, but i finally just copied your (slightly modified) code straight over. Hope that's okay with you! In recognition of your contribution, the DESC file now has this: https://github.com/ATFutures/dodgr/blob/fc1d8ce96ead8c61d0b58b5b62acafaf1960da7d/DESCRIPTION#L7 Thank you!