Update readme and move error #171
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maurolepore
approved these changes
May 26, 2021
After editing README.Rmd we need to render the corresponding README.md.
…ting/r2dii.plot.static into 151A-update-readme
|
@maurolepore - I changed some of your implementations and tests so I would appreciate you having a look at this PR now that it is ready for review. I also created a new issue #175 to add even more tests and checks for this function. |
maurolepore
approved these changes
May 26, 2021
Comment on lines
11
to
14
| scenario_specs <- data.frame( | ||
| scenario = c("sds", "sps", "cps", "sce4", "sce5"), | ||
| label = c("SDS", "STEPS", "CPS", "Scenario 4", "Scenario 5") | ||
| ) |
There was a problem hiding this comment.
In general I prefer prefer tibble() is it lacks the problematic argument stringsAsFactors and offers a more readable print() method.
Comment on lines
11
to
14
| scenario_specs <- data.frame( | ||
| scenario = c("sds", "sps", "cps", "sce4", "sce5"), | ||
| label = c("SDS", "STEPS", "CPS", "Scenario 4", "Scenario 5") | ||
| ) |
There was a problem hiding this comment.
Maybe name this too_many_scenarios and create another test with too_few_scenarios.
Comment on lines
2
to
9
| example_data <- process_input_data(example_data) | ||
|
|
||
| data <- prepare_for_trajectory_chart( | ||
| data = example_data, sector_filter = "power", technology_filter = "oilcap", | ||
| region_filter = "global", scenario_source_filter = "demo_2020", | ||
| value_name = "production", end_year_filter = 2025, | ||
| normalize_to_start_year = TRUE | ||
| ) |
There was a problem hiding this comment.
This is okay for now but as we add more tests it'd be nice to have a fake_trajectory_data() to avoid this complex setup -- which burries the intent of the test. So it'd become
too_many_scanarios <- tibble(...)
expect_snapshot_error(fake_trajectory_data(), too_many_scenarios, ...)
Comment on lines
4
to
5
| data <- prepare_for_trajectory_chart( | ||
| data = example_data, sector_filter = "power", technology_filter = "oilcap", |
There was a problem hiding this comment.
When I run devtools::test() I see:
Warning (test-plot_trajectory.R:4:3): with wrong number of scenarios errors gracefully
partial argument match of 'data' to 'data_preprocessed'You can use not data = ... but instead data_preprocessed = .... However, I think it's best to rename the argument to just data, and the function to prepare_for_trajectory() -- the word "chart" adds little value.
--
Crazy idea for the record:
I would be even more succinct and use prep_trajectory() (but I remember you prefer to keep the _for_). We could even go a step more formal and define the "trajectory" class and add the constructor new_trajectory() (as per this article). Then we could then implement print.trajectory() and call it as print(new_trajectory(data)).
|
I'd merge ASAP so that README works again and we |
|
I am going to merge this one is and we can address your comments about tests when we add more tests to this function. |
jdhoffa
added a commit
that referenced
this pull request
Sep 21, 2022
* 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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is a follow up on #166 which was merged too soon.
What it does: