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.

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

use math.js to evaluate user's arith expressions #31

Merged
merged 17 commits into from
Sep 25, 2020
Merged

use math.js to evaluate user's arith expressions #31

merged 17 commits into from
Sep 25, 2020

Conversation

yeedle
Copy link
Contributor

@yeedle yeedle commented Sep 21, 2020

as per the discussion in #29, this PR introduces the use of the math.js expression evaluator in the most secure way possible.

One important gotcha with math.js is that arrays are 1-indexed, so I had to update the R code to rescale the js bands between 1 and the length of the array. Another thing to take into account is that the syntax is both more expansive than standard javascript, but at the same time there are some subtle differences (like ^ being the exponent operator rather than the bitwise or operator). For more, see the documentation

@tim-salabim
Copy link
Member

Thanks. I will likely continue working on this over the weekend (thanks to the weather forecast:-) )

@tim-salabim
Copy link
Member

@yeedle, I'm sorry I had to revert this, but performance degradation was just too bad... Any ideas why this would degrade performance so much as compared to the with(Math) solution?

Here's an example you can try with the Math and with the math.js implementation to see what I mean. You need to zoom and pan around the image to see the degradation.

library(leaflet)
library(leafem)
library(stars)

tif = system.file("tif/L7_ETMs.tif", package = "stars")
x1 = read_stars(tif)
tmpfl = tempfile(fileext = ".tif")
x = st_warp(x1, crs = 4326)
write_stars(x, tmpfl)

pal = hcl.colors(256, "viridis")

ndvi = function(rst) (rst[4] - rst[3]) / (rst[4] + rst[3])
# ndvi = function(x) x[5]

leaflet() %>%
  addProviderTiles("Esri.WorldImagery") %>%
  addGeotiff(
    file = tmpfl
    , arith = ndvi
    # , bands = c(6, 5, 4)
    # , resolution = 30
    # , rgb = TRUE
    , opacity = 1
    , colorOptions = colorOptions(
      palette = pal
      # , na.color = "magenta"
      # , domain = c(-0.9, 0.9)
      # , breaks = seq(0, 255, 2)
    )
    # , pixelValuesToColorFn = NULL #myCustomJSFunc
    , group = "testNDVI"
  ) %>%
  addLayersControl(overlayGroups = "testNDVI")

@yeedle
Copy link
Contributor Author

yeedle commented Sep 25, 2020

Most probably it's because pixelValuesToColorFn runs the function once per pixel. the Function solution uses the built-in javascript parser (probably written in C) while math.js uses a JavaScript-based parser which is orders of magnitude slower as you can imagine.

If this is indeed the issue, the solution is to use math.js's compile function, which compiles the expression to JavaScript and you can then reuse the compiled expression. I'm going to test and see whether this approach helps, and if it does I'll submit a new PR using compile. Sounds good?

@tim-salabim
Copy link
Member

Sounds perfect, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants