andrewmarx / samc Goto Github PK

Although covered in the documentation, it would be beneficial to having a vignette dedicated to discussing disconnected graphs where examples can be shown visually. It would also be a place for more detailed explanations/visualizations as to why it matters for the cond_passage() functions.

Currently, short-term metrics only allow the input of a single integer for the time step parameter. This means that if users want to calculate a sequence of time steps like in the "Temporal analysis" tutorial, they must rerun the calculation from the start multiple times, which is inefficient. The time step parameter could be augmented to support a vector of integers so that the function is run only once and returns a list containing the multiple results.

Many users may prefer the use of the stars package over the raster package for managing raster data. Look into ways to seamlessly integrate stars support so that either can be used interchangeably.

Given that creating an samc object from raster data depends on the gDistance package internally, and that the gDistance package does not support stars objects, it's likely that any stars object would have to be converted to a raster layer. This would require creating and storing the entire raster layer in RAM, rather than reading the data as needed from disk. This may lead to memory concerns if the number of cells in a landscape is especially large (e.g. the data is very high res). However, the point at which this becomes a practical concern is unclear without testing, and it may be the case that computational limits become a limiting factor before the raster data reaches that size.

Questions to address:

• What data can be used (and how)?
• What are potential issues for data quality?

Add badges to the documentation:

• CRAN link: https://cran.r-project.org/package=samc
• Download count: https://cranlogs.r-pkg.org/badges/grand-total/samc

The ggplot2 website and readme provide an example to follow:
https://ggplot2.tidyverse.org/
https://github.com/tidyverse/ggplot2/blob/master/README.md

The above code is missing values = TRUE as an argument for compareRaster(). This results in checks for the occ parameter in various metrics to not catch mismatched NA cells, potentially leading to a crash (but not always). This issue will also occur if users attempt to directly use variants of the check() function where the first argument is a samc object.

Currently, in the "Animations" tutorial, using gifski with base plot produces a working output. The gganimate example technically works, but does not adjust the color scale across frames, resulting in a less useful visual. Unless gganimate adds built in support for dynamic color scales, a working animated ggplot example could be accomplished using gifski and ggplot, as mentioned in the tutorial's introduction.

When the package is loaded via the library() function, check the installed version and compare to the newest version available on CRAN. Output a startup message to the user informing them if a new version is available and how to update the package.

Currently, the samc() function only allows a single absorption input. In practice, users may want to parameterize multiple different sources of absorption (i.e., mortality). This could be implemented by supporting lists and RasterStacks for the absorption parameter. How the multiple sources of absorption would be treated by the package might be trickier. The simplest approach would be to assume that each source of absorption is mutually exclusive, in which case the probabilities are added together. I'm not sure if there is a practical scenario where users would not have mutually exclusive absorption events given that an individual cannot be "absorbed" more than once.

Gitter looks like it would be a convenient middle-ground between email and stackoverflow for support. It would also be a good way for people posting stackoverflow questions to get my attention.

Received this email from a CRAN maintainer:

You have Suggests: gifski but do not use it conditionally, as described
in §1.1.3.1 of 'Writing R Extensions'.

Please correct ASAP after CRAN submissions open on Jan 6 and before Jan
15 to safely retain the package on CRAN.

Relevant section in "Writing R Extensions": https://cran.r-project.org/doc/manuals/r-release/R-exts.html#Suggested-packages

A useful mailing list discussion for dealing with this: https://stat.ethz.ch/pipermail/r-package-devel/2018q1/002326.html

Another related mailing list discussion: https://stat.ethz.ch/pipermail/r-package-devel/2018q2/002780.html

A useful related blog post (from first mailing list link above): https://www.enchufa2.es/archives/suggests-and-vignettes.html

A vignette with the fix from the package discussed in the blog post: https://github.com/r-simmer/simmer/blob/master/vignettes/simmer-04-bank-2.Rmd

Trying to pass rasters via samc_obj$abs_states <- list(...) will produce an error. Instead, raster stacks should be used to pass raster layers. To make this more clear, update the code to check that the list elements are all matrices and produce a descriptive error message when they aren't

CRS objects from have a "comment" field not shown when using the raster::crs() function. This field represents the "well-known text" representation of the crs. Its construction/formatting can vary depending on the system and software versions used to construct the crs for the raster. The samc() function is not accounting for this difference when checking raster inputs.

The associated error is misleading; it was a piece of code that was copied/edited but the error message was not updated accordingly

Some links useful for the site landing page do not work when viewing the readme in the repository. The fix is to separate the site landing page content from the readme: https://pkgdown.r-lib.org/articles/pkgdown.html#home-page

Several functions use the same code to convert matrices to rasterlayers. This includes the samc() function, the check() function, and multiple metrics. To clean things up and improve maintainability, create an internal function to do this, and then update the other functions to use it.