densitymodelling / dsmextra

Extrapolation assessments in density surface models
GNU Lesser General Public License v3.0
4 stars 3 forks source link

2 bugs in dsmextra? #3

Closed dfifield closed 4 years ago

dfifield commented 4 years ago

Hi and Happy New Year!

Thanks for the great package - it's really helpful! I had started to cobble something much cruder together and so I was delighted when I found dsmextra!

I think I have found a couple of simple bugs. Apologies for not providing a complete reprex below, but I wasn't able to quickly pare down my segdata (327 rows) and predgrid (9124 rows) to make a minimal example that still displayed the behaviour in question.

1. Apply() sometimes returns the wrong type of structure

There are cases where apply() can return either a vector or matrix even though it was passed a dataframe, which messes up subsequent code.

There are two places in summarise_extrapolation.R where apply() is used to convert each column of a dataframe to character:

  1. At line 144: resdf <- apply(resdf, 2, function(x) as.character(x)), and
  2. At line 327: mic_resdf <- apply(mic_resdf, 2, function(x) as.character(x))

I have an example use case where apply() returns either a character vector or a 2-D character matrix on one of these rows instead of a dataframe, which messes up subsequent code that expects resdf or mic_resdf to be a dataframe.

For example, executing the following code where 'sst.sc' has no univariate or combinatorial extrapolation:

x <- compute_extrapolation(segments = segdata,
                           covariate.names = c("x.sc", "sst.sc"),
                           prediction.grid = predgrid,
                           coordinate.system = CRS("+proj=lcc +lon_0=-48 +lat_1=45.5 +lat_2=47 +ellps=GRS80"),
                           print.summary = TRUE,
                           save.summary = FALSE,
                           print.precision = 2)

causes the following output:

x <- compute_extrapolation(segments = segdata,
+                            covariate.names = c("x.sc", "sst.sc"),
+                            prediction.grid = predgrid,
+                            coordinate.system = CRS("+proj=lcc +lon_0=-48 +lat_1=45.5 +lat_2=47 +ellps=GRS80"),
+                            print.summary = TRUE,
+                            save.summary = FALSE,
+                            print.precision = 2)
Computing ...
Done!
 Show Traceback

 Rerun with Debug
 Error in rep("-----------", ncol(mic_resdf)) : invalid 'times' argument 

In this particular case, it fails at line 329 because apply() (at line 327) has returned a character vector and thus mic_resdf is no longer a dataframe at line 329.

Changing these two lines to use purrr::map_dfr() instead of apply() fixed the problem for me:

line 144: resdf <- purrr::map_dfr(resdf, as.character) and line 327: mic_resdf <- purrr::map_dfr(mic_resdf, as.character)

2. max.univariate misspelled as max.univariates

There are four places in compare_covariates.R (on lines 245 and 250) where max.univariates is used, but I think you intended it to be max.univariate?

Line 245: min.univariate <- c(min.univariate, rep("-", times = length(max.univariates)-length(min.univariate)))

Line 250: max.univariates <- c(max.univariates, rep("-", times = length(min.univariate)-length(max.univariates)))

In my use case, executing the following:

compare_covariates(extrapolation.type = "both",
                   segments = segdata,
                   n.covariates = NULL,
                   covariate.names = c("x.sc", "sst.sc"),
                   prediction.grid = predgrid,
                   coordinate.system = CRS("+proj=lcc +lon_0=-48 +lat_1=45.5 +lat_2=47 +ellps=GRS80"),
                   create.plots = TRUE,
                   display.percent = TRUE)

causes the following error:

Computing ...
|=============================================================================================================|100% ~0 s remaining     

Creating summaries ...
Error in compare_covariates(extrapolation.type = "both", segments = segdata,  : 
  object 'max.univariates' not found

Changing max.univariates to max.univariate on these two lines fixed the problem for me.

Thanks! Dave

pjbouchet commented 4 years ago

Hi Dave,

Happy New Year to you too!

Thank you very much for your kind words and the incredibly detailed bug report! This is fantastic, and I am glad you're finding the package useful.

I have released a new patch (v1.0.1) that implements your fixes above.

Much appreciated. Best, Phil