148 fix target sda outputs #153

jdhoffa · 2020-07-29T14:09:48Z

The original target_sda function had several places in which the output was unexpected. This was a result of basing the function off of a methodology document with errors.

In this PR, I have already:

updated the vignettes/target_sda.Rmd to reflect the correct methodology (and reworded the article to make it more clear)
temporarily removed regression tests that reflect potentially incorrect output
added on LARGE regression test, that reflects the correct output (including a new possibility to output several targets for several scenarios)
work on target_sda() until that large test passes
refactor that test into smaller more meaningful tests

Relates to #144, #145, #148

ref values are wrong to begin with, so i am commenting these out while I fix the code.

I will work on the code until this massive expect known_output test passes. After this, I will refactor the test into more meaningful chunks.

jdhoffa · 2020-07-29T14:10:32Z

tests/testthat/test-target_sda.R

+test_that("with known input outputs as expected", {
+  #TODO: Refactor this test into smaller isolated expected output tests
+matched <- fake_matched(sector_ald = "cement")
+
+ald <- fake_ald(
+        sector = "cement",
+        technology = "cement",
+        name_company = c(rep("shaanxi auto", 4), "company 2"),
+        year = c(2020, 2021, 2022, 2025, 2020),
+        emission_factor = c(0.9, 0.9, 0.8, 0.5, 12)
+      )
+
+
+co2_intensity_scenario <- fake_co2_scenario(
+  scenario = c(rep("b2ds", 2), rep("sds", 2)),
+  year = rep(c(2020, 2025), 2),
+  emission_factor = c(0.5, 0.1, 0.5, 0.4)
+
+)
+
+out <- target_sda(matched,
+                  ald,
+                  co2_intensity_scenario) %>%
+  arrange(.data$year)
+
+out <- split(out, out$emission_factor_metric)
+
+expected_out <- tibble::tribble(
+                   ~sector, ~year,  ~emission_factor_metric, ~emission_factor_value,
+                  "cement", 2020L,              "projected",                    0.9,
+                  "cement", 2021L,              "projected",                    0.9,
+                  "cement", 2022L,              "projected",                    0.8,
+                  "cement", 2025L,              "projected",                    0.5,
+                  "cement", 2020L,            "target_b2ds",                    0.9,
+                  "cement", 2025L,            "target_b2ds",                   1.29,
+                  "cement", 2020L,             "target_sds",                    0.9,
+                  "cement", 2025L,             "target_sds",                   5.16,
+                  "cement", 2020L, "adjusted_scenario_b2ds",                   6.45,
+                  "cement", 2025L, "adjusted_scenario_b2ds",                   1.29,
+                  "cement", 2020L,  "adjusted_scenario_sds",                   6.45,
+                  "cement", 2025L,  "adjusted_scenario_sds",                   5.16,
+                  "cement", 2020L,      "corporate_economy",                   6.45,
+                  "cement", 2021L,      "corporate_economy",                    0.9,
+                  "cement", 2022L,      "corporate_economy",                    0.8,
+                  "cement", 2025L,      "corporate_economy",                    0.5
+                  )
+
+
+expect_equal(out$projected$emission_factor_value, c(0.9, 0.9, 0.8, 0.5))
+expect_equal(out$corporate_economy$emission_factor_value, c(06.45, 0.9, 0.8, 0.5))
+expect_equal(out$adjusted_scenario_b2ds$emission_factor_value, c(6.45, 1.29))
+expect_equal(out$adjusted_scenario_sds$emission_factor_value, c(6.45, 5.16))
+expect_equal(out$target_b2ds$emission_factor_value, c(0.9, 1.29))
+expect_equal(out$target_sds$emission_factor_value, c(0.9, 5.16))
+
+
+})


This is the test I am aiming to have pass

jdhoffa · 2020-07-30T16:12:28Z

@maurolepore there are a few TODO's, that I wanted your input on, and still that tibble R CMD CHECK warning, but other than that, this is more or less ready to be reviewed.

maurolepore · 2020-07-30T16:54:47Z

tests/testthat/test-target_sda.R

+
+out <- split(out, out$emission_factor_metric)
+
+expected_out <- tibble::tribble(


This is why R CMD check complained. We import dplyr::tibble(), not tibble::tibble().

We could import tibble in DESCRIPTION, then @importFrom tibble tibble; but we already depend on dplyr, and it reexports tibble() -- so we use dplyr::tibble(). This is confusing.

maurolepore · 2020-07-30T20:19:50Z

tests/testthat/test-target_sda.R

+expected_out <- tribble(
+   ~sector, ~year,  ~emission_factor_metric, ~emission_factor_value,
+  "cement", 2020L,              "projected",                    0.9,
+  "cement", 2021L,              "projected",                    0.9,
+  "cement", 2022L,              "projected",                    0.8,
+  "cement", 2025L,              "projected",                    0.5,
+  "cement", 2020L,            "target_b2ds",                    0.9,
+  "cement", 2025L,            "target_b2ds",                   1.29,
+  "cement", 2020L,             "target_sds",                    0.9,
+  "cement", 2025L,             "target_sds",                   5.16,
+  "cement", 2020L, "adjusted_scenario_b2ds",                   6.45,
+  "cement", 2025L, "adjusted_scenario_b2ds",                   1.29,
+  "cement", 2020L,  "adjusted_scenario_sds",                   6.45,
+  "cement", 2025L,  "adjusted_scenario_sds",                   5.16,
+  "cement", 2020L,      "corporate_economy",                   6.45,
+  "cement", 2021L,      "corporate_economy",                    0.9,
+  "cement", 2022L,      "corporate_economy",                    0.8,
+  "cement", 2025L,      "corporate_economy",                    0.5
+  )


Is this dead code? expected_out seems unused.

Yup you are correct! Thanks

jdhoffa · 2020-07-31T12:38:46Z

Just had a look, and all the changes look good to me. Once we sort the regression value issues out after the team meeting, lets merge this :)

…dhoffa/r2dii.analysis into 148-enhance-target_sda-outputs

jdhoffa added 5 commits July 29, 2020 13:17

fix incorrect methodology in article

1505540

mend

08ed7ec

temporarily block regression tests

b3ba78c

ref values are wrong to begin with, so i am commenting these out while I fix the code.

add one giant regression test

cee953d

I will work on the code until this massive expect known_output test passes. After this, I will refactor the test into more meaningful chunks.

mend

c46f963

jdhoffa commented Jul 29, 2020

View reviewed changes

jdhoffa added 2 commits July 29, 2020 16:54

start year calculated correctly, for multiple sectors and scenarios

1bc5679

rename py as p to align with methodology

ecdda15

jdhoffa added 5 commits July 30, 2020 15:57

rename all function arguments as data

a81a8e6

SERIOUS refactoring to target_sda

832df1f

add tests to warn if no match in input data

bf4c015

tidy existing tests

8f5c6a3

R CMD CHECK passes for target_sda

2d38c8f

jdhoffa marked this pull request as ready for review July 30, 2020 16:10

remove stray comments

7a80cad

maurolepore added 2 commits July 30, 2020 11:51

Update regression references

636e465

Fix tibble

88fba61

maurolepore reviewed Jul 30, 2020

View reviewed changes

merge upstream/master

243b106

maurolepore reviewed Jul 30, 2020

View reviewed changes

Style

69e21c2

This was linked to issues Jul 31, 2020

Output names of target_* are not clear #144

Closed

Output of target_sda should include adjusted_scenario and corporate economy #148

Closed

Avoid warning

c7eb3fb

Store regression references from Docker

2b1c913

maurolepore approved these changes Jul 31, 2020

View reviewed changes

jdhoffa added 2 commits July 31, 2020 16:39

update NEWS.md

324a665

Merge branch '148-enhance-target_sda-outputs' of https://github.com/j…

e169fe2

…dhoffa/r2dii.analysis into 148-enhance-target_sda-outputs

jdhoffa merged commit 41fe44d into RMI-PACTA:master Jul 31, 2020

jdhoffa deleted the 148-enhance-target_sda-outputs branch August 3, 2020 09:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

148 fix target sda outputs #153

148 fix target sda outputs #153

jdhoffa commented Jul 29, 2020 •

edited

Loading

jdhoffa Jul 29, 2020

jdhoffa commented Jul 30, 2020

maurolepore Jul 30, 2020 •

edited

Loading

maurolepore Jul 30, 2020

jdhoffa Jul 31, 2020

jdhoffa commented Jul 31, 2020


		out <- split(out, out$emission_factor_metric)

		expected_out <- tibble::tribble(

148 fix target sda outputs #153

148 fix target sda outputs #153

Conversation

jdhoffa commented Jul 29, 2020 • edited Loading

jdhoffa Jul 29, 2020

Choose a reason for hiding this comment

jdhoffa commented Jul 30, 2020

maurolepore Jul 30, 2020 • edited Loading

Choose a reason for hiding this comment

maurolepore Jul 30, 2020

Choose a reason for hiding this comment

jdhoffa Jul 31, 2020

Choose a reason for hiding this comment

jdhoffa commented Jul 31, 2020

jdhoffa commented Jul 29, 2020 •

edited

Loading

maurolepore Jul 30, 2020 •

edited

Loading