Add qplots for techmix and emission intensity #364
Conversation
| "The `technology_share` values are plotted for extreme years. | ||
| Do you want to plot different years? E.g. filter the data with:\\ | ||
| `subset(data, year %in% c(2020, 2030))`." | ||
| )) |
There was a problem hiding this comment.
I am not sure if this is not an overkill.
There was a problem hiding this comment.
Do you intend to inform both when span_5yr = TRUE and span_5yr = FALSE?
There was a problem hiding this comment.
Should we enhance the message with the user's data? E.g.
- replacing the literal "extreeme years" with the actual years we are plotting.
- replacing
datawith the symbol the user used to name their data -- see how by searching for calls similar to.data <- deparse_1(substitute(data, env = env)) - replacing
"... c(2020, 2030) ..."with"... {range(data$year, na.rm = TRUE)} ..."
There was a problem hiding this comment.
For messages I usually like the "-ing" form. Also, from this message I'm not sure why it's important for me (a user) to know this informaiton. If I understand correctly the issue we want the user to be aware is that they'll get plot for a range of years that is different than the standard 5-years span defined by PACTA, right?
If so, here is my suggestion:
span <- diff(range(data$year, na.rm = TRUE))
if (span > 5L) {
inform(glue("Plotting `technology_share` values for more than the 5 years span defined by PACTA".))
}
if (span < 5L) {
... Plotting ... for less than the 5 years defined by PACTA ...
}
R/plot_techmix.R
Outdated
| data %>% | ||
| select(.data$technology, .data$label_tech) %>% | ||
| unique(), | ||
| by = "technology") |
There was a problem hiding this comment.
I need the label_tech from 'data'. If there is any cleaner way to get it - I'd like to know!
| "The `technology_share` values are plotted for extreme years. | ||
| Do you want to plot different years? E.g. filter the data with:\\ | ||
| `subset(data, year %in% c(2020, 2030))`." | ||
| )) |
There was a problem hiding this comment.
Do you intend to inform both when span_5yr = TRUE and span_5yr = FALSE?
|
|
||
| --- | ||
|
|
||
| [1] "Tons of CO2 per ton" |
There was a problem hiding this comment.
Tons of Co2 per ton <of what?>
There was a problem hiding this comment.
Good question! We need a unit!
| @@ -0,0 +1,4 @@ | |||
| # Has the title as expected | |||
|
|
|||
| Current and future technology mix for the Power sector | |||
There was a problem hiding this comment.
The letter case seems odd: It's sentence case until we reach "Power". A more conventional way to emphasize a word is with italics or "quotes", or simply by making it take the emphasis position of a sentence -- the position right next to a colon, semi-colon, or period.
Suggestions:
- Current and future technology mix for the power sector
- Current and future technology mix for the "power" sector
- Current and future technology mix for the sector power.
| expect_equal(pretty, metrics) | ||
| metrics <- unique(p$data$label) | ||
| ugly <- c("projected", "corporate_economy") | ||
| expect_equal(ugly, metrics) |
There was a problem hiding this comment.
I think we need to pass ugly to the expected argument of expect_equal(), i.e.
expect_equal(actual = metrics, expected = ugly)This will result in clearer failure messages. See how actual and expected are mixed up here
There was a problem hiding this comment.
Yes, I did this wrong in many places :-)
There was a problem hiding this comment.
😆 alright will try to amend
| expect_equal(p$labels$x, "Year") | ||
| expect_snapshot_output(p$labels$x) | ||
|
|
||
| expect_match(p$labels$y, "[Tt]ons of CO2 per ton") |
There was a problem hiding this comment.
I often use [Aa] because I want to ignore the letter case of A. But in this case we likely want this test to fail if "A" becomes "a", right?
There was a problem hiding this comment.
Yes, true. In this case it is not the right way to do it
tests/testthat/test-qplot_techmix.R
Outdated
| ) | ||
| p <- qplot_techmix(data) | ||
|
|
||
| expect_true(max(p$data$year) - min(p$data$year) <= 5) |
There was a problem hiding this comment.
Do we need na.rm = TRUE? We haven't tested the package when data has NA, so I fear we'll see bugs where we call min(), max(), range(), sum(), and every other function that includes na.rm.
In this PR I add quick plots for:
Also - some refactoring is performed.