`technology_share` for `target_*` is now properly calculated and weighted #294

jdhoffa · 2021-05-10T12:51:31Z

The target_* values of technology_share should be calculated as follows (in line with the methodology):
1 - Calculate the production_target for each company (without weighting)
2 - Calculate the technology_share_target for each company (without weighting)
3 - Apply the portfolio_weight to each of production, technology_share, production_target and technology_share_target

This has been achieved.
@maurolepore this is an MVP solution, as this has been an open bug for some time. Please review (and note that I recognize that some of my solutions are not so elegant).

Also @georgeharris2deg if you could please have a look at this it would be much appreciated.

I would rather we get the solution out the door, and we can refactor later.

Closes #277

maurolepore

LGTM. I have a few questions you may consider and merge.

NEWS.md

R/summarize_weighted_production.R

R/target_market_share.R

jdhoffa · 2021-05-11T11:37:05Z

R/summarize_weighted_production.R

+add_technology_share_target <- function(data) {
+  crucial <- c("production_target", "sector_ald", "year", "technology")
+
+  check_crucial_names(data, crucial)
+  walk_(crucial, ~ check_no_value_is_missing(data, .x))
+
+  data %>%
+    group_by(.data$sector_ald, .data$year, .data$scenario, .data$name_ald) %>%
+    mutate(technology_share_target = .data$production_target / sum(.data$production_target)) %>%
+    group_by(!!!dplyr::groups(data))
+}
+


this more or less duplicates add_technology_share. Good candidate for refactoring

jdhoffa · 2021-05-11T11:38:39Z

R/target_market_share.R

-  if (weight_production) {
-    data <- summarize_weighted_production(
-      data,
-      !!!rlang::syms(summary_groups),
-      use_credit_limit = use_credit_limit
-    )
-  } else {
-    data <- summarize_unweighted_production(
-      data,
-      !!!rlang::syms(summary_groups)


This now happens AFTER adding the targets. (See line 231). I also had to expand the summarize_weighted_production function, as it now needs to implement something different.

A lot of WET code here unfortunately.

jdhoffa · 2021-05-11T11:40:09Z

R/target_market_share.R

+    data <- data %>%
+      ungroup() %>%
+      add_loan_weight(use_credit_limit = use_credit_limit) %>%
+      add_technology_share() %>%
+      add_technology_share_target() %>%
+      calculate_weighted_loan_metric("production") %>%
+      calculate_weighted_loan_metric("technology_share") %>%
+      calculate_weighted_loan_metric("production_target") %>%
+      calculate_weighted_loan_metric("technology_share_target") %>%
+      group_by(
+        .data$sector_ald,
+        .data$technology,
+        .data$year,
+        !!!rlang::syms(summary_groups)
+      ) %>%
+      summarize(
+        weighted_production = sum(.data$weighted_loan_production),
+        weighted_technology_share = sum(.data$weighted_loan_technology_share),
+        weighted_production_target = sum(.data$weighted_loan_production_target),
+        weighted_technology_share_target = sum(.data$weighted_loan_technology_share_target)
+      ) %>%
+      # Restore old groups
+      group_by(!!!dplyr::groups(data))
+  } else {
+    data <- data %>%
+      select(-c(
+        .data$id_loan,
+        .data$loan_size_credit_limit,
+        .data$loan_size_outstanding
+      )) %>%
+      distinct() %>%
+      group_by(.data$sector_ald, .data$technology, .data$year, !!!rlang::syms(summary_groups)) %>%
+      # FIXME: Confusing: `weighted_production` holds unweighted_production?
+      summarize(
+        weighted_production = .data$production,
+        weighted_production_target = .data$production_target,
+        .groups = "keep"
+      ) %>%
+      ungroup(.data$technology) %>%
+      mutate(
+        weighted_technology_share = .data$weighted_production / sum(.data$weighted_production),
+        weighted_technology_share_target = .data$weighted_production_target / sum(.data$weighted_production_target)
+      ) %>%
+      group_by(!!!dplyr::groups(data))
+  }
+


These dplyr chains are essentially summarize_weighted_production and summarize_unweighted_production, but expanded. The different is that they now need to summarize both the technology_share and technology_share_target. Unfortunately, I couldn't change summarize_weighted_production itself that much, as it is an exported function.

One possibility is to add an argument to summarize_weighted_production that allows you to include targets or not. @maurolepore happy to explore options.

jdhoffa · 2021-05-11T11:41:23Z

tests/testthat/test-target_market_share.R

-    technology = c("electric", "ice", "electric", "ice", "electric", "ice", "electric", "ice"),
-    year = c(2020, 2020, 2021, 2021, 2020, 2020, 2021, 2021),
-    tmsr = c(1, 1, 1.85, 0.6, 1, 1, 1.85, 0.6),
-    smsp = c(0, 0, 0.34, -0.2, 0, 0, 0.34, -0.2)
+    technology = c("electric", "ice", "electric", "ice"),
+    year = c(2020, 2020, 2021, 2021),
+    tmsr = c(1, 1, 1.85, 0.6),
+    smsp = c(0, 0, 0.34, -0.2)


This was a poorly written test, since the scenario was effectively duplicated.
That said, perhaps it's good to write a test to see what happens if the scenario inputs duplicated scenarios?
I think the function should pass an error if this is the case, as I wouldn't know how to handle that otherwise.

tests/testthat/test-target_market_share.R

maurolepore · 2021-05-12T12:21:34Z

Thanks @jdhoffa for your comments. I see a bunch of things we should follow up on and I worry that left only here they will fall through the cracks. I suggest we do either of these:

Do them now
Move them to issues
Write TODO comments in code

The order reflects my preference. You still have the context in your head and getting it done now seems the cheapest. This issue has already waited for a long time, and the number of impacted users I believe was low.

jdhoffa · 2021-05-12T12:53:35Z

Happy to do them now, except unfortunately I am in calls all afternoon, and tomorrow is a holiday, so I will have to get to it on Friday.

This reverts commit c7a851d.

This reverts commit 473265d.

jdhoffa · 2021-05-18T13:59:43Z

R/summarize_weighted_production.R

+  summarize_weighted_production_(data, ..., use_credit_limit = use_credit_limit, add_targets = FALSE)
+}
+
+summarize_weighted_production_ <- function(data, ..., use_credit_limit = FALSE, add_targets = FALSE) {


I have added this new internal function summarize_weighted_production_ which contains the argument add_targets. I then call summarize_weighted_production with default values for that internal function (this is to preserve the existing arguments of summarize_weighted_production).

@maurolepore let me know what you think of this solution as a way to avoid WET code

maurolepore

I see some CI workflows fail

maurolepore · 2021-05-18T14:47:10Z

R/summarize_weighted_production.R

    # FIXME: Confusing: `weighted_production` holds unweighted_production?
-    summarize(weighted_production = .data$production, .groups = "keep") %>%
-    ungroup(.data$technology, .data$tmsr, .data$smsp) %>%
-    mutate(weighted_technology_share = .data$weighted_production / sum(.data$weighted_production)) %>%
-    group_by(!!!dplyr::groups(data))
+
+  if (add_targets) {
+    data %>%
+      summarize(
+        weighted_production = .data$production,
+        weighted_production_target = .data$production_target,
+        .groups = "keep"
+      ) %>%
+      ungroup(.data$technology) %>%
+      mutate(
+        weighted_technology_share = .data$weighted_production / sum(.data$weighted_production),
+        weighted_technology_share_target = .data$weighted_production_target / sum(.data$weighted_production_target)


The #FIXME comment is now misplaced. Maybe we can remove it and instead write a comment explaining why we use the "weighted_" prefix in this function? Or change the prefix for a temporary one?

More accurate name, since this flag doesn't actually add anything just checks to see if targets are already there or not.

jdhoffa · 2021-05-19T08:45:21Z

Exploring the failing CI checks right now. It seems they are on older R releases only?

jdhoffa · 2021-05-19T14:44:14Z

R/target_market_share.R

@@ -300,7 +300,7 @@ target_market_share <- function(data,
  ald_with_benchmark <- calculate_ald_benchmark(ald, region_isos, by_company)

  data %>%
-    rbind(ald_with_benchmark) %>%
+    dplyr::bind_rows(ald_with_benchmark) %>%


well hot diggity!

jdhoffa added 7 commits May 10, 2021 13:46

calculate targets prior to summarizing

02a588d

add function to deal with tech_share_targets

f27edf8

add failing test

6fce3fe

prevent double counting over region

54509b7

update expected values with correct tech_share

4e01902

style

697e9b0

remove old FIXME

876420c

jdhoffa requested a review from maurolepore May 10, 2021 12:51

jdhoffa added 2 commits May 10, 2021 15:03

update NEWS.md

a45838a

update NEWS.md

f995b66

maurolepore approved these changes May 11, 2021

View reviewed changes

NEWS.md Outdated Show resolved Hide resolved

R/summarize_weighted_production.R Outdated Show resolved Hide resolved

R/target_market_share.R Outdated Show resolved Hide resolved

jdhoffa commented May 12, 2021

View reviewed changes

fix NEWS.md

2a3aeae

jdhoffa added 9 commits May 14, 2021 15:47

knit readme

16c0deb

sum with narm TRUE

473265d

check crucials at exported fn

c7a851d

Revert "check crucials at exported fn"

394e1d0

This reverts commit c7a851d.

Revert "sum with narm TRUE"

6c5214d

This reverts commit 473265d.

refactor summarize_*_targets

049e1c9

refactor: combine summarize-like functions

1dcc4e8

check crucials at exported fn

f1647c5

dont change exported function

66b797a

jdhoffa requested a review from maurolepore May 18, 2021 13:33

jdhoffa commented May 18, 2021

View reviewed changes

maurolepore approved these changes May 18, 2021

View reviewed changes

maurolepore self-requested a review May 18, 2021 14:49

jdhoffa added 2 commits May 19, 2021 09:43

add_targets -> with_targets

d1dcd69

More accurate name, since this flag doesn't actually add anything just checks to see if targets are already there or not.

move FIXME and make more descriptive

e6beae1

Replace rbind() with dplyr::bind_rows()

5c1cd07

jdhoffa commented May 19, 2021

View reviewed changes

maurolepore merged commit c302426 into RMI-PACTA:master May 19, 2021

jdhoffa deleted the 277-target_before_weight branch May 19, 2021 15:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`technology_share` for `target_*` is now properly calculated and weighted #294

`technology_share` for `target_*` is now properly calculated and weighted #294

jdhoffa commented May 10, 2021

maurolepore left a comment

jdhoffa May 11, 2021

jdhoffa May 11, 2021

jdhoffa May 11, 2021

jdhoffa May 11, 2021

maurolepore commented May 12, 2021

jdhoffa commented May 12, 2021

jdhoffa May 18, 2021

maurolepore left a comment •

edited

Loading

maurolepore May 18, 2021

jdhoffa commented May 19, 2021

jdhoffa May 19, 2021

technology_share for target_* is now properly calculated and weighted #294

technology_share for target_* is now properly calculated and weighted #294

Conversation

jdhoffa commented May 10, 2021

maurolepore left a comment

Choose a reason for hiding this comment

jdhoffa May 11, 2021

Choose a reason for hiding this comment

jdhoffa May 11, 2021

Choose a reason for hiding this comment

jdhoffa May 11, 2021

Choose a reason for hiding this comment

jdhoffa May 11, 2021

Choose a reason for hiding this comment

maurolepore commented May 12, 2021

jdhoffa commented May 12, 2021

jdhoffa May 18, 2021

Choose a reason for hiding this comment

maurolepore left a comment • edited Loading

Choose a reason for hiding this comment

maurolepore May 18, 2021

Choose a reason for hiding this comment

jdhoffa commented May 19, 2021

jdhoffa May 19, 2021

Choose a reason for hiding this comment

`technology_share` for `target_*` is now properly calculated and weighted #294

`technology_share` for `target_*` is now properly calculated and weighted #294

maurolepore left a comment •

edited

Loading