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

Improve debugging and error messages for report functions #604

Closed
orichters opened this issue May 27, 2024 · 5 comments
Closed

Improve debugging and error messages for report functions #604

orichters opened this issue May 27, 2024 · 5 comments
Labels
enhancement New feature or request

Comments

@orichters
Copy link
Contributor

orichters commented May 27, 2024

Dear colleagues,
I find it sometimes really annoying and difficult to find out where and why a remind2 function call fails exactly, because mostly somewhere a mbind files. Often, the report functions have the following structure:

cons <- gdx::readGDX(gdx,name=c("vm_cons"),field="l",restore_zeros=FALSE,format="first_found")
out <- NULL
out <- mbind(out, setNames(cons, "Consumption"))

Now, if I do some stupid stuff, I get this in the logs:

out <- mbind(out, setNames(cons["LAM",,], "Consumption LAM"))

Error in `mbind()`:
! Cannot handle objects! Data as well as spatial dimensions differ!
Run `rlang::last_trace()` to see where the error occurred.

And often, also rlang::last_trace() is not really helpful, in particular, if the var name was not typed in as here but rather composed of some sets.

I thought we could maybe replace it by something like that which would give a much more consistent and meaningful error message:

bindName <- function(out, data, name) {
  joined <- try(mbind(out, setNames(data, name)))
  if (inherits(joined, "try-error")) {
    stop(paste(c(paste0("Failing mbind for variable '", name, "':"),
                 "### out:",
                 capture.output(str(out)),
                 "### new data:",
                 capture.output(str(setNames(data, name)))
                ), collapse = "\n"))
  }
  return(joined)
}

And then use:

out <- bindName(out, cons["LAM",,], "Consumption LAM")

Error in mbind(out, setNames(data, name)) :
  Cannot handle objects! Data as well as spatial dimensions differ!
Error in `bindName()`:
! Failing mbind for variable 'Consumption LAM'
### out:
A magpie object (package: magclass)
 @ .Data:  num [1:228] 1.649 1.422 0.673 8.218 0.966 ...
 $ dimnames:List of 3
  ..$ all_regi: chr [1:12] "LAM" "OAS" "SSA" "EUR" ...
  ..$ ttot    : chr [1:19] "y2005" "y2010" "y2015" "y2020" ...
  ..$ data    : chr "Consumption"
### new data:
A magpie object (package: magclass)
 @ .Data:  num [1:19] 1.65 1.81 2.05 2.28 2.53 ...
 $ dimnames:List of 3
  ..$ all_regi: chr "LAM"
  ..$ ttot    : chr [1:19] "y2005" "y2010" "y2015" "y2020" ...
  ..$ data    : chr "Consumption LAM"
Run `rlang::last_trace()` to see where the error occurred.

Now I see directly which variable was affected and what was the issue (all_regi differs).

Maybe you have even better ideas, @0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q, @Renato-Rodrigues, @fbenke-pik.

@orichters orichters added the enhancement New feature or request label May 27, 2024
@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member

It is a bit hit and miss. Somebody would have to convert the ~500 usages of that idiom in the package,. Then users would have to stick to it. But the ~450 other uses of mbind() would not benefit.

The solution is obvious: proper error reporting by mbind() itself, instead of a cooked-up wrapper function.

@fbenke-pik
Copy link
Contributor

What Michaja said. Have you tried creating a PR to magclass?

@orichters
Copy link
Contributor Author

orichters commented May 27, 2024

No, my only experience with "magclass" is: "replacing all occurrences by quitte".

@fbenke-pik
Copy link
Contributor

https://github.com/pik-piam/magclass/blob/master/R/mbind.R#L79-L81

I guess we could make these error messages more meaningful?

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member

https://github.com/pik-piam/magclass/blob/master/R/mbind.R#L79-L81

I guess we could make these error messages more meaningful?

The problem is: to return really meaningful error messages, one needs to make some assumptions about the use mbind() is put to. Which might or might not jive with Jan.
Specifically, one hast to assume that few regions/years/names are bound to many regions/years/names, that the latter are the first inputs element, and that only the differences to the first inputs element are significant.
I have a draft, I just need to polish it a bit before submitting it to the court of RSE opinion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants