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.

Sign up for GitHub

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

Jump to bottom

Rename plot timeline and add missing test #279

Merged
merged 5 commits into from
Jun 16, 2021
Merged

Rename plot timeline and add missing test #279

merged 5 commits into from
Jun 16, 2021

Conversation

MonikaFu
Copy link
Collaborator

I rename the plot_timeline files to `plot_emission_intensity'

@MonikaFu MonikaFu marked this pull request as draft June 16, 2021 15:02
@MonikaFu MonikaFu changed the title Rename plot timeline Rename plot timeline and add missing test Jun 16, 2021
@MonikaFu MonikaFu marked this pull request as ready for review June 16, 2021 15:31
maurolepore
maurolepore approved these changes Jun 16, 2021
View reviewed changes
Copy link
Contributor

@maurolepore maurolepore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except we need stringsAsFactors = FALSE or tibble(). Other comments are NB.

R/plot_emission_intensity.R Outdated
"Can't plot more than {n_colours} lines. Found {n_lines} lines:
{toString(data$line_name)}."
"Can't plot more than {max_n_lines} lines in one plot. Found {n_lines} lines:
{toString(data$line_name)}. Consider splitting over multiple plots."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice for the hint to start in a new line.

tests/testthat/_snaps/plot_emission_intensity.md Outdated
# with too many lines to plot errors gracefully

Can't plot more than 7 lines in one plot. Found 12 lines:
Projected, Corporate Economy, Target Demo, Adjusted Scenario Demo, 1, 2, 3, 4, 5, 6, 7, 8. Consider splitting over multiple plots.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to reflow so the structure is

Problem.
Actual.
Hint.

So

Can't plot more than 7 lines in one plot. 
Found lines: Projected, Corporate Economy, ...
Consider splitting (<what?/how?>) over multiple plots.

tests/testthat/test-plot_emission_intensity.R Outdated
min_year <- min(data$year)
max_year <- max(data$year)
for (i in 1:n) {
fake_data <- data.frame(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use stringsAsFactors = FALSE or tibble() -- else we may get different behaviour across versions of R.

maurolepore
maurolepore approved these changes Jun 16, 2021
View reviewed changes
Copy link
Contributor

@maurolepore maurolepore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@maurolepore maurolepore merged commit d84c585 into develop Jun 16, 2021
@maurolepore maurolepore deleted the rename-plot-timeline branch June 16, 2021 16:45
jdhoffa added a commit that referenced this pull request Sep 21, 2022
@jdhoffa @maurolepore
@MonikaFu @actions-user
Release/0.0.1 (#492)
517a8d2 
* Export example_data (#127)

* In plot_timeline() remove labels (#128)

* Remove dependency on ggpubr (#152)

* Clarify and simplify messages and tests (#154)

* In plot_timeline(), order legend to match lines (#158)

* New versions of plot_timeline() (#156)

* Enforce atomic sector and warn selection (#148)

* Show how to change colours (#162)

* Update readme (#164)

* In prepare_for_timeline() sector_filter is now obligatory  (#163)

* New versions of prepare_for_timeline*() (#165)

* update readme (#167)

* Add workflow to render all rmarkdown documents (#174)

* Update readme and move error  (#171)

* theme_2dii() now includes common arguments (#168)

* Fix gh action to render readme (#181)

* Update wordslist (#182)

* Rename package (#184)

* Move process_input_data() into prepare_*() functions (#176)

* Revert "Move process_input_data() into prepare_*() functions (#176)"

* Rename datasets (#186)

* prepare_for_ -> prep_ (#188)

* Fix names of data (#191)

* Polish file names and prune unused files (#192)

* Move process_input_data() inside prep_*() (#194)

* Use colours as data, and name them consistently (#189)

* Annotate lines instead of a legend (#183)

* Remove '.static' from package name (#195)

* Show theme set (#190)

* Style (#196)

* Structure website's reference section (#197)

* Colour datasets are now internal (#199)

* Add trajectory version B  (#201)

* Fix timeline (#204)

* Remove filtering ability from prep functions (#206)

* Refactor for consistency and polish documentation (#207)

* Add main_line argument trajectory (#210)

* Fix error message (#214)

* Remove needless exapmle-chunk (#217)

* Prune thick API (#222)

* Fix annotations overlap (#221)

* Rename thick API with suffix "Y" (#224)

* prep_timeline() again gains 'sector_filter' (#225)

* prep_timelineY() again starts at common start year (#227)

* plot_techmix*() now enforce a single scenario (#229)

* plot_trajectoryX() looses arg 'value' for consistency with X API (#231)

* New articles for X and Y API (#232)

* Filter year to make plots of X and Y API identical (#236)

* Add example Y techmix (#238)

* Align timeline examples (#239)

* show similarities pros and cons (#240)

* Align X and Y articles for techmix and trajectory (#241)

* dry high level docs (#246)

* DRY again and polish text (#247)

* Dry-run of CRAN release (#251)

* Polish DESCRIPTION Title and Description (#252)

* Redocument (#253)

* New abort_if_multiple() hints users how to filter (#254)

* GH workflows now work with branch 'develop' (#257)

* Trigger CI on PR too (#258)

* Make the X API the main API (#256)

* Fix website reference (#272)

* Rename to plot_emission_intensity() (#273)

* 268 In examples use base r (#274)

* Add authors and roles (#248)

* Rename plot timeline and add missing test (#279)

* Remove main_line (#280)

* plot_trajectory() loses the argumet 'normalize' (#281)

* Don't beatifull lengend labels (#284)

* Style (#286)

* All plots again beautify legend labels by default (#293)

* Update DESCRIPTION's Title and Description (#296)

* Rename timeline to emission intensity, eveverywhere (#295)

* Filter data to startyear (#290)

* Remove dead code and increase test coverage (#297)

* Handle messages so they don't pollute test output (#298)

* Rename to emission intensity (#299)

* Refactor (#300)

* 294 Remove extrapolate (#301)

* In internal code use dplyr::filter() not subset() (#302)

* 271 New article of entire r2dii workflow (#303)

* New integration test for sda (#304)

* Polish documentation (#308)

* Rebuild readme (#309)

* Improve messages by showing the start year (#306)

* Refactor (#310)

* Simplify get_area_borders code (#311)

* Fix maintainer's email (#312)

* Prepare for initial release 0.0.1 (#314)

* Spellcheck (#316)

* Update readme (#317)

* Increment version number (#318)

Co-authored-by: Mauro Lepore <maurolepore@gmail.com>
Co-authored-by: Monika Furdyna <monika.furdyna@gmail.com>
Co-authored-by: Mauro Lepore <mauro@2degrees-investing.org>
Co-authored-by: GitHub Actions <actions@github.com>
Co-authored-by: Alex Axthelm <Alex@2degrees-investing.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maurolepore maurolepore maurolepore approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MonikaFu @maurolepore