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

use piamInterfaces to generate mappings #1016

Merged
merged 2 commits into from
Oct 24, 2022

Conversation

orichters
Copy link
Contributor

@orichters orichters commented Oct 17, 2022

Purpose of this PR

  • refactor xlsx_IIASA.R, called via output.R -> export
  • if no mapping file is provided, generate one using piamInterfaces
  • improves the use of iiasatemplate such that also units are checked and possibly corrected
  • write more to the log file about what has been done
  • @pfuehrlich-pik: do I have to add piamInterfaces elsewhere to the dependencies? I added a line to the renv tutorial as well that I found useful
  • requires add checkIIASASubmission pik-piam/piamInterfaces#14

Type of change

(Make sure to delete from the Type-of-change list the items not relevant to your PR)

  • Refactoring
  • no impact on default scenarios

Checklist:

  • My code follows the coding etiquette
  • I have performed a self-review of my own code


meslog <- function(text, LOGFILE) {
message(text)
write(text, file = LOGFILE, append = TRUE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also sink which can achieve a similar thing: Run sink(LOGFILE, append = TRUE, type = "message", split = TRUE), then all calls to message will write to terminal (actually stderr) and the logfile. You must remember to call sink() at the end of the script/when you want to stop redirecting output to the logfile.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

split = TRUE only splits R output (via Rvprintf) and the default output from writeLines: it does not split all output that might be sent to stdout. (https://stat.ethz.ch/R-manual/R-devel/library/base/html/sink.html)

So I don't think it works here…

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be, although in my experience sink usually works as expected. I'm a little afraid that we will see meslog or similar functions defined over and over in different scripts, in that case I'd much prefer pulling meslog up into e.g. the gms package.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please try sink(file("test.log", "w"), type = "message", split = TRUE) (without the file part, it also crashes):

Error in `sink()`:
! cannot split the message connection

Happy to add meslog or something similar to gms.

Copy link
Contributor

@pfuehrlich-pik pfuehrlich-pik Oct 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I'm convinced sink does not like messages. I just remembered another way:

withCallingHandlers({
  # all the code that should message to console and log
}, message = function(x) {
  cat(x$message, file = LOGFILE, append = TRUE)
  message(x$message) # not sure, maybe need to use cat here
})

This is better than the sink approach anyway, because you cannot forget to run sink() at the end. This is also similar to what madrat does.

DESCRIPTION Outdated
@@ -48,6 +48,7 @@ Imports:
ncdf4,
nleqslv,
optparse,
piamInterfaces (>= 0.0.14),
Copy link
Contributor

@pfuehrlich-pik pfuehrlich-pik Oct 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the right way to declare the dependency, yes.

@orichters orichters force-pushed the xlsx_IIASA branch 3 times, most recently from 8176ffc to 8b6ba51 Compare October 24, 2022 13:12

writexl::write_xlsx(list("data" = writetoexcel), OUTPUT_xlsx)
}, message = function(x) {
cat(x$message, file = logFile, append = TRUE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this also still print to console or only to logFile?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it prints also to console. I don't know exactly, why, to be fair, but what you suggested produced two prints to the console.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay good. I'd guess there's a chain of handlers, and after your custom handler it will just continue with the default handler which prints to console.

@orichters orichters merged commit faaf99d into remindmodel:develop Oct 24, 2022
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.

None yet

3 participants