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 correctly orders scale_colour_r2dii input #528

Merged
merged 10 commits into from
Jan 23, 2024

Conversation

jdhoffa
Copy link
Member

@jdhoffa jdhoffa commented Jan 17, 2024

@jacobvjk has pointed out that, if a user passes a custom colour scaling to plot_emission_intensity, along with specific levels of emission_factor_metric to which the scaling should be applied, the result doesn't appear with correct colours.

This is caused by an internal function, match_lines_order that forcibly changes the levels of the dataset to a custom order (which the user of the function can't know arbitrarily). This means that custom scalings are impossible to achieve consistently, as they will always be applied to the factor levels as ordered within the function.

This function was introduced to solve (what seems like) a cosmetic problem, and removing it will likely only have the minor consequence of the legend colour orders not being identical the line colour orders, as they appear on the plot (a UI issue, but not a functional one). See discussion in #527

To fix this:

  • I got rid of match_lines_order
  • I changed an argument name from labels to colour_labels. The original argument clashed with a required argument name to be passed in through ...

Closes #527

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

jdhoffa commented Jan 23, 2024

PTAL: We should look into these deprecation warnings, there's something funky going on there, see #530

@jdhoffa jdhoffa requested a review from MonikaFu January 23, 2024 11:19
@jdhoffa jdhoffa marked this pull request as ready for review January 23, 2024 11:19
@jdhoffa jdhoffa requested a review from jacobvjk January 23, 2024 12:50
@MonikaFu
Copy link
Collaborator

LGTM. Also tested locally and works as expected. As I mentioned in #527 I would like to try to bring back the ordering of the legend (using a different solution that does not impact the order of the data itself). Will you make an issue about it or should I?

@jdhoffa
Copy link
Member Author

jdhoffa commented Jan 23, 2024

#534 is the issue, feel free to edit it as you see fit :-)

@jdhoffa jdhoffa merged commit 9e043e7 into main Jan 23, 2024
22 checks passed
@jdhoffa jdhoffa deleted the 527-plot_emission_intensity_with_correct_colours branch January 23, 2024 14:49
@jacobvjk
Copy link
Member

@jdhoffa one comment on this. I understood the test as an implied order of colours mapped to categories. I.e. "projected" should be "dark_blue". Running this locally does not return projected as dark blue.

So just to be sure: does this fix aim to allow mapping the exact pairs of categories to colours outside the function? If so, great. If the idea was to produce the mapping that I would say the test implies, I don't seem to be able to transfer this to the workflow I am using this on.

@jdhoffa
Copy link
Member Author

jdhoffa commented Jan 23, 2024

Oh man, ok. Let me have a look tomorrow, maybe i messed something up.

@jdhoffa
Copy link
Member Author

jdhoffa commented Jan 24, 2024

@jacobvjk can you try this

data <- filter(sda, sector == "cement", region == "global") %>% 
  dplyr::mutate(
    emission_factor_metric = factor(
      .data$emission_factor_metric, 
      levels = c(
        "projected", 
        "corporate_economy", 
        "target_demo", 
        "adjusted_scenario_demo"
      )
    )
  )

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

p <- plot_emission_intensity(data)

p <- p + scale_colour_r2dii(
  colour_labels = input_color_scale,
)

@jdhoffa jdhoffa mentioned this pull request Feb 21, 2024
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 this pull request may close these issues.

plot_emission_intensity outputs metrics with incorrect order of colours when used with scale_colour_r2dii
3 participants