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

plot_emission_intensity outputs metrics with incorrect order of colours when used with scale_colour_r2dii #527

Closed
jdhoffa opened this issue Jan 17, 2024 · 6 comments · Fixed by #528
Assignees
Labels
bug an unexpected problem or unintended behavior

Comments

@jdhoffa
Copy link
Member

jdhoffa commented Jan 17, 2024

Expected behaviour:
Using plot_emission_intensity with input data that has specifically ordered factors for emission_factor_metric in conjunction with scale_colour_r2dii with custom colour labels, outputs a plot whos emission_factor_metric has those colours (same order).

Observed behaviour:
Output has the correct values for colour, however the order gets scrambled.

See reprex below:

library(dplyr)
#> 
#> Attaching package: 'dplyr'
#> The following objects are masked from 'package:stats':
#> 
#>     filter, lag
#> The following objects are masked from 'package:base':
#> 
#>     intersect, setdiff, setequal, union
library(r2dii.plot)

data <- sda %>%
  filter(region == "global")

input_levels <- c(
  "projected",
  "corporate_economy",
  "target_demo",
  "adjusted_scenario_demo"
)

input_color_scale <-
  c(
    "dark_blue",
    "green",
    "grey",
    "ruby_red"
  )

input_color_scale_hex <- data.frame(label = input_color_scale) %>%
  left_join(palette_colours, by = "label") %>%
  pull(hex)

expected_output <- data.frame(
  levels = input_levels,
  hex = input_color_scale_hex
)

p <- data %>%
  dplyr::mutate(
    emission_factor_metric = factor(
      .data$emission_factor_metric,
      levels = input_levels
    )
  ) %>%
  plot_emission_intensity() +
  scale_colour_r2dii(
    labels = input_color_scale
  )
#> Warning: The `data` argument of `plot_emission_intensity()` must be prepped already as
#> of r2dii.plot 0.4.0.
#> ℹ From the next release you will need to call
#>   `r2dii.plot::plot_emission_intensity(data)` prior to calling
#>   `r2dii.plot::plot_emission_intensity()`.
#> ℹ Alternatively custom data preparation will also become possible.
#> This warning is displayed once every 8 hours.
#> Call `lifecycle::last_lifecycle_warnings()` to see where this warning was
#> generated.
#> Scale for colour is already present.
#> Adding another scale for colour, which will replace the existing scale.

# print the levels that colours are applied to
ordered_output_levels <- levels(r2dii.plot:::match_lines_order(p$data))

# print the actual colour scales of the plot
ordered_output_colour_scale <- p$scales$get_scales("colour")$palette(
  length(ordered_output_levels)
)

plot_output <- data.frame(
  levels = ordered_output_levels,
  hex = ordered_output_colour_scale
)

# Note that `corporate_economy` and `projected` are swapped
plot_output
#>                   levels     hex
#> 1      corporate_economy #1b324f
#> 2              projected #00c082
#> 3            target_demo #d0d7e1
#> 4 adjusted_scenario_demo #a63d57
expected_output
#>                   levels     hex
#> 1              projected #1b324f
#> 2      corporate_economy #00c082
#> 3            target_demo #d0d7e1
#> 4 adjusted_scenario_demo #a63d57

Created on 2024-01-17 with reprex v2.0.2

@jdhoffa jdhoffa added the bug an unexpected problem or unintended behavior label Jan 17, 2024
@jdhoffa jdhoffa self-assigned this Jan 17, 2024
@jdhoffa
Copy link
Member Author

jdhoffa commented Jan 23, 2024

The function match_lines_order() in plot_emission_intensity forcefully factors the input dataset, before passing it to the colour parameter of the aes() function.
What this means is that, regardless of how the data is factored prior to input, it may be factored differently on output.
This also means that, if a user wants to input a custom scale order, they must use the SAME order of levels as the factor defined by this function. However, since this happens functionally, the user is left to guess what this will be.

match_lines_order <- function(data) {

@jdhoffa
Copy link
Member Author

jdhoffa commented Jan 23, 2024

Function was introduced in #339 however this was a pure refactor, and the logic existed before

@jdhoffa
Copy link
Member Author

jdhoffa commented Jan 23, 2024

#158 (from 3 years ago, seemingly one of the first commits to this function) is where this behavior was introduced.

@jdhoffa
Copy link
Member Author

jdhoffa commented Jan 23, 2024

@MonikaFu I would like to revert some of the logic that was introduced in that PR.

In my opinion, it is more preferred that the user should be able to reliably input specific levels and colours than it is for the legend to match the order of colours on the line plot. One is a functional difference in API, the other is cosmetic.

(it seems you from 3 years ago agreed with me hahaha):
#158 (review)

Note: Mauro demos that it is possible to change the colours still with the forcats approach here: #158 (comment) , however it doesn't take into account the possibility of many lines with potentially different factor levels

@MonikaFu
Copy link
Collaborator

MonikaFu commented Jan 23, 2024

I agree with you on this

In my opinion, it isp preferred that the user should be able to reliably input specific levels and colours than it is for the legend to match the order of colours on the line plot.

That is why I called this behaviour a bug. However, I do think that the legend order matching the order of the lines helps the plot readibility a lot, especially when more lines are present. I'd like to research if it is possible to change order of the legend only without changing the order of the data. We can do it in another issue.

@jdhoffa
Copy link
Member Author

jdhoffa commented Jan 23, 2024

Sounds great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants