pik-piam / magclass

R package | Data Class and Tools for Handling Spatial-Temporal Data
GNU Lesser General Public License v3.0
4 stars 24 forks source link

ideosyncratic behaviour of [<- method leads to hard to track bugs #149

Closed 0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q closed 1 year ago

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented 1 year ago

We had this bug (pik-piam/mrremind#403)

-  weights[, c(2005, 2010, 2015, 2020)] <- iea
+  weights[, c(2005, 2010, 2015, 2020), ] <- iea

in the mrremind package lately, which invalidated (part of) the REMIND input data.

This is a bug that is easily made (it's just one character) and easily overlooked during review (it's just one character). But also something that we can only track by chance. For one thing, the last REMIND input data generation had a grand total of 17393 warnings:

$ tar -Oxf /p/projects/rd3mod/inputdata/output/rev6.54APT-2023-06-08_62eff8f7_remind.tgz ./diagnostics.log | awk '$0 ~ /WARNING/ { sub(/^~* */, "", $0); a[$0]++ } END { PROCINFO["sorted_in"] = "@val_num_desc"; for (b in a) printf("% 6d: %s\n", a[b], b) }'
 11640: WARNING: an argument will be fractionally recycled
  4760: WARNING: collapsing to unique 'x' values
   532: WARNING: 
   190: WARNING: steady-state not reached
   144: WARNING: no non-missing arguments to max; returning -Inf
    56: WARNING: Column 'pkm' does not exist to remove
    56: WARNING: Column 'weight' does not exist to remove
     2: WARNING: NAs introduced by coercion
     2: WARNING: Data returned by mrremind:::calcIndustry_Value_Added(...)
     2: WARNING: Data returned by mrremind:::calcCement() contains NAs
     1: WARNING: More than 50% missing values for: OAS
     1: WARNING: Product/flow combinations not present in mapping added by 
     1: WARNING: setConfig must not be used from within retrieveData!
     1: WARNING: Data returned by mrcommons:::calcMAgPIEReport(...) contains NAs
     1: WARNING: number of items to replace is not a multiple of replacement length
     1: WARNING: Data for following unknown country codes removed: XAN
     1: WARNING: Data returned by mrremind:::calcJRC_IDEES(...) contains NAs
     1: WARNING:  - toolCountryFill set missing values for IMPORTANT countries to 0:
     1: WARNING: Data returned by mrremind:::calcEmissionFactors(...) contains NAs

only one of which (to the best of our knowledge) had a negative influence on the input data.

For another, should the "number of items to replace" by chance be "a multiple of replacement length", there is no warning, but the result may still be wrong.

Two questions:

  1. Since the warning itself is not issued by magclass itself, is this behaviour considered a bug or a feature of magpie objects?
  2. Can we get an option to deactivate this behaviour? I don't think it is intentionally used (for mrremind and related packages). I'm positive it should not be used. So such an option would allow us to ferret out all instances where it is used and change them, then prevent bugs like this in the future, without breaking any legacy code that might use it (intentionally or not).

A couple of examples:

> foo <- new.magpie(1:3, 1:3)
> 
> # works as intended
> foo[,1:2,] <- 1:6; foo
An object of class "magpie"
, , 1

              year
region.region1 y0001 y0002 y0003
         GLO.1     1     4    NA
         GLO.2     2     5    NA
         GLO.3     3     6    NA

> 
> # does the wrong thing, issues a warning
> foo[,1:2] <- 1:6; foo
Warning message:
In x@.Data[i] <- k :
  number of items to replace is not a multiple of replacement length
An object of class "magpie"
, , 1

              year
region.region1 y0001 y0002 y0003
         GLO.1     1     4     1
         GLO.2     2     5     2
         GLO.3     3     6     3

> 
> # does the wrong thing, keeps silent
> foo[,1:2] <- 1:3; foo
An object of class "magpie"
, , 1

              year
region.region1 y0001 y0002 y0003
         GLO.1     1     1     1
         GLO.2     2     2     2
         GLO.3     3     3     3

> 
> # throws an error
> foo[,1:2,] <- 1:3; foo
Error in .local(x, i, j, ..., value) : 
  Replacement does not work! Different replacement length!
tscheypidi commented 1 year ago

I close this one as the core problem is discussed in #150