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

create_format_terra_vect: temp fix for issue with replacing function body and {covr} #39

Closed
wants to merge 1 commit into from

Conversation

brownag
Copy link
Contributor

@brownag brownag commented Mar 16, 2024

This is a workaround for replacement of a function body not working properly within the evaluation environment created by covr, targets tests, and testthat.

More commentary in new issue #38

overwrite = TRUE,
options = NULL
options = geotargets::geotargets_option_get("gdal.vector.creation_options")
)
}
body(.write_terra_vector)[[2]][["filetype"]] <- filetype
Copy link
Owner

@njtierney njtierney Mar 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we even actually need to modify the function body directly? I'm fairly sure those parts will be evaluated appropriately?

E.g.,

example_funfun <- function(sep_type){
  .write_fun <- function(object, file){
    # loudly declare the quote type when writing
    readr::write_lines(x = object,
                       file = file,
                       sep = sep_type)
    cat("Hi! The separation marker is: '", sep_type,"'", sep = "")
    cat("\n")
    cat("We wrote the file out to:\n", file, sep = "")
  }
  
  return(
    list(
      read = readr::read_csv,
      write = .write_fun
    )
  )
  
}

my_example <- example_funfun(",")

# sep_type isn't evaluated yet
my_example$write
#> function(object, file){
#>     # loudly declare the quote type when writing
#>     readr::write_lines(x = object,
#>                        file = file,
#>                        sep = sep_type)
#>     cat("Hi! The separation marker is: '", sep_type,"'", sep = "")
#>     cat("\n")
#>     cat("We wrote the file out to:\n", file, sep = "")
#>   }
#> <environment: 0x12a2669e0>

# but sep_type gets evaluated later when the function is used:
my_example$write(
  object = "1,2,3,4", 
  file = tempfile(fileext = ".csv")
  )
#> Hi! The separation marker is: ','
#> We wrote the file out to:
#> /var/folders/9c/k3wqmhhx4qsb3fd66n4prhlw0000gq/T//RtmpUdvv1a/file104a0120aafb7.csv

Created on 2024-03-16 with reprex v2.1.0

Session info
sessioninfo::session_info()
#> ─ Session info ───────────────────────────────────────────────────────────────
#>  setting  value
#>  version  R version 4.3.3 (2024-02-29)
#>  os       macOS Sonoma 14.3.1
#>  system   aarch64, darwin20
#>  ui       X11
#>  language (EN)
#>  collate  en_US.UTF-8
#>  ctype    en_US.UTF-8
#>  tz       Australia/Hobart
#>  date     2024-03-16
#>  pandoc   3.1.1 @ /Applications/RStudio.app/Contents/Resources/app/quarto/bin/tools/ (via rmarkdown)
#> 
#> ─ Packages ───────────────────────────────────────────────────────────────────
#>  package     * version date (UTC) lib source
#>  bit           4.0.5   2022-11-15 [1] CRAN (R 4.3.0)
#>  bit64         4.0.5   2020-08-30 [1] CRAN (R 4.3.0)
#>  cli           3.6.2   2023-12-11 [1] CRAN (R 4.3.1)
#>  crayon        1.5.2   2022-09-29 [1] CRAN (R 4.3.0)
#>  digest        0.6.34  2024-01-11 [1] CRAN (R 4.3.1)
#>  evaluate      0.23    2023-11-01 [1] CRAN (R 4.3.1)
#>  fansi         1.0.6   2023-12-08 [1] CRAN (R 4.3.1)
#>  fastmap       1.1.1   2023-02-24 [1] CRAN (R 4.3.0)
#>  fs            1.6.3   2023-07-20 [1] CRAN (R 4.3.0)
#>  glue          1.7.0   2024-01-09 [1] CRAN (R 4.3.1)
#>  hms           1.1.3   2023-03-21 [1] CRAN (R 4.3.0)
#>  htmltools     0.5.7   2023-11-03 [1] CRAN (R 4.3.1)
#>  knitr         1.45    2023-10-30 [1] CRAN (R 4.3.1)
#>  lifecycle     1.0.4   2023-11-07 [1] CRAN (R 4.3.1)
#>  magrittr      2.0.3   2022-03-30 [1] CRAN (R 4.3.0)
#>  pillar        1.9.0   2023-03-22 [1] CRAN (R 4.3.0)
#>  pkgconfig     2.0.3   2019-09-22 [1] CRAN (R 4.3.0)
#>  purrr         1.0.2   2023-08-10 [1] CRAN (R 4.3.0)
#>  R.cache       0.16.0  2022-07-21 [2] CRAN (R 4.3.0)
#>  R.methodsS3   1.8.2   2022-06-13 [2] CRAN (R 4.3.0)
#>  R.oo          1.26.0  2024-01-24 [2] CRAN (R 4.3.1)
#>  R.utils       2.12.3  2023-11-18 [2] CRAN (R 4.3.1)
#>  R6            2.5.1   2021-08-19 [1] CRAN (R 4.3.0)
#>  readr         2.1.5   2024-01-10 [1] CRAN (R 4.3.1)
#>  reprex        2.1.0   2024-01-11 [2] CRAN (R 4.3.1)
#>  rlang         1.1.3   2024-01-10 [1] CRAN (R 4.3.1)
#>  rmarkdown     2.25    2023-09-18 [1] CRAN (R 4.3.1)
#>  rstudioapi    0.15.0  2023-07-07 [1] CRAN (R 4.3.0)
#>  sessioninfo   1.2.2   2021-12-06 [2] CRAN (R 4.3.0)
#>  styler        1.10.2  2023-08-29 [2] CRAN (R 4.3.0)
#>  tibble        3.2.1   2023-03-20 [1] CRAN (R 4.3.0)
#>  tidyselect    1.2.0   2022-10-10 [1] CRAN (R 4.3.0)
#>  tzdb          0.4.0   2023-05-12 [1] CRAN (R 4.3.0)
#>  utf8          1.2.4   2023-10-22 [1] CRAN (R 4.3.1)
#>  vctrs         0.6.5   2023-12-01 [1] CRAN (R 4.3.1)
#>  vroom         1.6.5   2023-12-05 [1] CRAN (R 4.3.1)
#>  withr         3.0.0   2024-01-16 [1] CRAN (R 4.3.1)
#>  xfun          0.42    2024-02-08 [1] CRAN (R 4.3.1)
#>  yaml          2.3.8   2023-12-11 [1] CRAN (R 4.3.1)
#> 
#>  [1] /Users/nick/Library/R/arm64/4.3/library
#>  [2] /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library
#> 
#> ──────────────────────────────────────────────────────────────────────────────

Or is there some deeper stuff happening with targets here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that will work, or at least I tried it before arriving at the body<- solution I proposed in #11 here. Tried doing it again now because I hoped I had missed something.

What you propose works fine interactively in your global environment, or in a reprex, etc. but the context where those tar_format() functions get evaluated you will not have the e.g. sep_type object defined.

Essentially the functions tar_format() uses need to be self-contained, not relying on the prior parent frame. You actually want sep_type to be evaluated sooner as it is when we modify the function body.

If you implement your suggestion for tar_terra_rast() you will get something like this:

Loading required namespace: terra
▶ dispatched target test_terra_rast
✖ errored target test_terra_rast
✖ errored pipeline [0.13 seconds]
Error:
! Error running targets::tar_make()
Error messages: targets::tar_meta(fields = error, complete_only = TRUE)
Debugging guide: https://books.ropensci.org/targets/debugging.html
How to ask for help: https://books.ropensci.org/targets/help.html
Last error message:
    _store_ object 'filetype' not found
Last error traceback:
    No traceback available.
# tar-terra-rast.R: create_format_terra_raster
#...
 .myfun <- function(filetype, gdal) {
        .write_terra_raster <- function(object, path) {
            terra::writeRaster(
                object,
                path,
                filetype = filetype,
                overwrite = TRUE,
                gdal = gdal, 
            )
        }
        return(list(write = .write_terra_raster))
    }

    targets::tar_format(
        read = function(path) terra::rast(path),
        write = .myfun(filetype, gdal)$write,
        marshal = function(object) terra::wrap(object),
        unmarshal = function(object) terra::unwrap(object)
    )
# ...

Some other ideas I tried were using purrr::partial() to compose functions with some arguments pre-specified and couldn't get that working. Also forcing evaluation and injecting values with {rlang} metaprogramming operators also did not appear to work, but that might be worth re-investigating now.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, thanks for explaining that! It looks like we lose all reference to the parent frames here, I guess it might be evaluated in a separate R process, so making changes directly to the body of the function makes sense. I'm surprised that metaprogramming things from {rlang} don't work out, it feels like this is the sort of thing where you want to change some aspect of the AST or something. But effectively, I think, that's what you're doing with the clever body()<- calls.

If it continues to be a problem we can re-evaluate some possible solutions with rlang - I might even tag in Will into the issues to see if he has had this problem?

@brownag
Copy link
Contributor Author

brownag commented Mar 19, 2024

We now have a better way to do this, that does not require body<- nor getting package options inside write function body

@brownag brownag closed this Mar 19, 2024
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