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

2 bugs in dsmextra? #3

Closed
dfifield opened this issue Jan 2, 2020 · 1 comment
Closed

2 bugs in dsmextra? #3

dfifield opened this issue Jan 2, 2020 · 1 comment

Comments

@dfifield
Copy link

dfifield commented Jan 2, 2020

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 added a commit that referenced this issue Jan 8, 2020
Fix to issue #3 (2 bugs in dsmextra) posted by Dave Fifield on Jan 2, 2020
@pjbouchet
Copy link
Collaborator

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

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

No branches or pull requests

2 participants