pik-piam / mrremind

The mrremind packages contains data preprocessing for the REMIND model.
GNU Lesser General Public License v3.0
4 stars 43 forks source link

Fix warnings #503

Closed fbenke-pik closed 4 months ago

fbenke-pik commented 4 months ago

This fixes two obnoxious warnings in inputdata generation

  1. This type of warnings is spamming the log and has its origin in readGGDC10: Expecting numeric in O7550 / R7550C15: got 'incl in gov. services'
  2. Apparently madrat does static code analysis when loading it and throws a misleading warning when you call toolGetMapping with parameters, as done previously in calcIO. Therefore, I rewrote it to a more explicit (but more redundant) version that madrat should understand (cc @0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q )
0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented 4 months ago

Apparently madrat does static code analysis when loading it and throws a misleading warning when you call toolGetMapping with parameters, as done previously in calcIO. Therefore, I rewrote it to a more explicit (but more redundant) version that madrat should understand (cc @0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q )

Say whaaat? madrat happens to magically do static code analysis, that nobody put in there? And that triggers on one

$ tar -Oxf /p/projects/rd3mod/inputdata/output/rev6.75APT-2024-05-18_62eff8f7_remind.tgz ./diagnostics.log \
  | grep 'when calling toolGetMapping from within a madrat function'
~~~~~ argument 'where' should be set when calling toolGetMapping from within a madrat function.

out of six

$ tar -Oxf /p/projects/rd3mod/inputdata/output/rev6.75APT-2024-05-18_62eff8f7_remind.tgz ./diagnostics.log \
  | grep 'Run calcOutput(type = "IO"' \
  | grep -o 'subtype = "[^"]*"' \
  | sort | uniq
subtype = "IEA_output"
subtype = "input"
subtype = "output"
subtype = "output_biomass"
subtype = "output_Industry_subsectors"
subtype = "trade"

instances of the same call structure?

Magic indeed.

:point_right: #505

fbenke-pik commented 4 months ago

Say whaaat? madrat happens to magically do static code analysis, that nobody put in there?

Loading madrat previously produced this warning 1: In .warnNotfound(getMappings) : Mapping in mrremind:::calcIO not found!

The corresponding code can be found here: https://github.com/pik-piam/madrat/blob/master/R/getCode.R

getCode is called by R/getMadratGraph.R.

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented 4 months ago
  1. When would one ever see those, instead of the cheery There were 50 or more warnings (use warnings() to see the first 50) when working with madrat? :confused: Nonetheless, the warning did exist, and I feel silly for my comment. On the up side, one less warning.
  2. Looking at madrat::getCode(), I think this change sets a bad precedent. In my opinion it is not helpful to try to reason about code by evaluating it (which by the way is the opposite of static code analysis) separate from its execution environment. All instances of .warnNotfound(getMappings) I found (four times mredgebuildings, and mrremind::calcIO()) are false negatives where the mapping name is composited. This encourages repetitive, therefore redundant, therefore error-prone code.
fbenke-pik commented 4 months ago

When would one ever see those, instead of the cheery There were 50 or more warnings (use warnings() to see the first 50) when working with madrat?

The long-term goal is to get rid of all of the warnings, so the log and warning messages become meaningful and useful again. We hope to tackle all low-hanging fruit after the merge of the new EDGE-Transport version.

This particular warning came to my attention when running madrat functions locally and was confusing me for a while.

Looking at madrat::getCode(), I think this change sets a bad precedent.

I agree with you, just was too lazy to set up a madrat issue for this. But maybe it is worth opening an issue and bring this to Jan's attention.

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented 4 months ago

:point_right: pik-piam/madrat#209