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

issue #307, replace RandomFields #314

Merged
merged 11 commits into from
May 10, 2023
Merged

issue #307, replace RandomFields #314

merged 11 commits into from
May 10, 2023

Conversation

DanWismer
Copy link
Contributor

No description provided.

@DanWismer DanWismer requested a review from edwardsmarc May 9, 2023 19:48
@codecov-commenter
Copy link

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.03 🎉

Comparison is base (2e115c0) 71.52% compared to head (a53a5a1) 71.55%.

❗ Current head a53a5a1 differs from pull request most recent head 95155ac. Consider uploading reports for the commit 95155ac to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #314      +/-   ##
==========================================
+ Coverage   71.52%   71.55%   +0.03%     
==========================================
  Files          76       76              
  Lines        6661     6669       +8     
==========================================
+ Hits         4764     4772       +8     
  Misses       1897     1897              
Impacted Files Coverage Δ
R/fct_simulate_excludes.R 100.00% <ø> (ø)
R/fct_simulate_includes.R 100.00% <ø> (ø)
R/fct_simulate_themes.R 100.00% <ø> (ø)
R/fct_simulate_weights.R 100.00% <ø> (ø)
R/fct_write_project.R 98.14% <ø> (ø)
R/fct_simulate_spatial_data.R 100.00% <100.00%> (ø)
R/utils_raster_io.R 38.46% <100.00%> (ø)
R/utils_spatial_io.R 92.30% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@edwardsmarc
Copy link
Contributor

Checks

@DanWismer my check passed with 1 WARNING about the raster package being built under a different version of R. I also got a few a NOTES:

> checking package dependencies ... NOTE
  Imports includes 47 non-default packages.
  Importing from so many packages makes the package vulnerable to any of
  them becoming unavailable.  Move as many as possible to Suggests and
  use conditionally.

> checking installed package size ... NOTE
    installed size is 29.5Mb
    sub-directories of 1Mb or more:
      app       6.5Mb
      extdata  16.9Mb
      help      4.1Mb

> checking dependencies in R code ... NOTE
  Namespaces in Imports field not imported from:
    'R.utils' 'cachem' 'future' 'ipc' 'markdown' 'pkgload' 'plyr' 'rcbc'
    'rgdal' 'withr'
    All declared Imports should be used.

> checking R code for possible problems ... [22s] NOTE
  simulate_themes: no visible global function definition for 'na.omit'
  simulate_weights : <anonymous>: no visible global function definition
    for 'na.omit'
  Undefined global functions or variables:
    na.omit
  Consider adding
    importFrom("stats", "na.omit")
  to your NAMESPACE file.

I'm not sure if we need to address that last one in this branch?

terra

The terra update for downloading rasters appears to work. But the behavior when you select multiple rasters to download is odd. It only ever downloads a single tif called data.tif, and if you have multiple rasters selected it adds them as different bands in data.tif. Is this the standard behavior? It does the same thing with the current published version of the app. I was expecting to get a tif for each layer I selected in the download widget.

@DanWismer
Copy link
Contributor Author

Thanks @edwardsmarc !

We can address the notes on a different PR. These notes have been the same for sometime now.

For the data download, I believe this is the same behavior as before (I will check Sites to confirm). Instead of exporting individual .tifs, it exports the stack of rasters as a single multiband tif. If you open it up in Arc Catalog, you can drag in the individual rasters:

image

And each download has the same name, data.tif

@jeffreyhanson
Copy link
Contributor

jeffreyhanson commented May 10, 2023

Just a heads up, the warning no visible global function definition for 'na.omit' can be addressed by replacing na.omit with stats::na.omit. It's saying that there's no na.omit function in the base package (or those listed under Depends in the DESCRIPTION), so you just need to tell it which package to look in (using ::).

@DanWismer
Copy link
Contributor Author

Just a heads up, the warning no visible global function definition for 'na.omit' can be addressed by replacing na.omit with stats::na.omit.

Thanks Jeff! I was able to work in your prioritizr code for simulating data.

@jeffreyhanson
Copy link
Contributor

Ah - awesome!

@DanWismer
Copy link
Contributor Author

DanWismer commented May 10, 2023

Interesting, on sites, when you download, it names the multi band rasters as Band1, Band2, Band3 etc.
image

I assume terra handles this better and actually assigns the raster name to the band?

@jeffreyhanson
Copy link
Contributor

Yeah, IIRC using terra::writeRaster will preserve the layer names (unlike raster::writeRaster).

@edwardsmarc
Copy link
Contributor

Thanks @edwardsmarc !

We can address the notes on a different PR. These notes have been the same for sometime now.

For the data download, I believe this is the same behavior as before (I will check Sites to confirm). Instead of exporting individual .tifs, it exports the stack of rasters as a single multiband tif. If you open it up in Arc Catalog, you can drag in the individual rasters:

image

And each download has the same name, data.tif

Thanks for explanation @DanWismer. This makes sense, and I'm getting the same result: the terra version keeps the layer names but the raster version does not.

@DanWismer DanWismer merged commit 46a5e60 into master May 10, 2023
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.

4 participants