plot_techmix and plot_trajectory gain more specific checks

[jdhoffa]

• plot_* functions gain check for the presence of the label column
• Brought to light a bug in a qplot function that was fixed
• stray env arg removed from a prep_ function that didn't need it

Closes #548

[MonikaFu]

See my comments regarding prep_trajcetory(). It might also be a good idea to go through the documentation of each function prep_*(), plot_*(), qplot_* for both trajectory and techmix and see if we have checks for each requirement listed there.

[jdhoffa]

Note to self: go through the documentation of all functions to ensure sufficient input checks:

• prep_trajectory and plot_trajectory and qplot
• prep_techmix and plot_techmixand qplot
• prep_emission_intensity and plot_emission_intensityand qplot

[jdhoffa]

@MonikaFu All documented expectations should now be well represented in code for all prep, plot and qplot style functions.

except for:

r2dii.plot/R/prep_techmix.R

Lines 7 to 8 in 5210ebc

	#' * The column `metric` must have a portfolio (e.g. "projected"), a benchmark
	#' (e.g. "corporate_economy"), and a single `scenario` (e.g. "target_sds").

There is a check for single scenarios, but I haven't seen any checks anywhere for the presence of "a portfolio and benchmark" on input. Does that part of the documentation require a "bare minimum" check or is it an exception?

Note: The package also had no checks for the input parameters convert_label or span_5yr on any of the functions, so I added some very simple checks there. You may improve them in a future PR if you like, but at least they are covered now.

Note: I was waffling back and forth between how to refactor the input checks of the qplot_ functions (since they really should be identical to the requirements to the prep_ functions, just with default arguments). If you have a preference there, let me know.

[jdhoffa]

Note: Windows checks seem to be failing due to the breaking change to r2dii.data that introduced the new increasing_or_decreasing dataset.

I don't really understand why only the windows checks fail, since in principle the same checks should be applied to R-CMD-check/macos-latest... in any case, I believe it is unrelated to this PR.

[MonikaFu]

There is a check for single scenarios, but I haven't seen any checks anywhere for the presence of "a portfolio and benchmark" on input. Does that part of the documentation require a "bare minimum" check or is it an exception?

Indeed, it should be checking for presence of "projected" value in metric column. We missed that. The benchmark is optional. I propose to reword it in the documentation to:

• The column metric must have a "projected" value and
  a single scenario (e.g. "target_sds").
• (Optional) The column metric may contain a benchmark
  (e.g. "corporate_economy").

And check if "projected" is in the metric column. The presence of scenario is checked in abort_if_multiple_scenarios(). To be consistent with what the function actually does we should maybe rename it to abort_if_wrong_number_of_scenarios() or something like that.

Note: The package also had no checks for the input parameters convert_label or span_5yr on any of the functions, so I added some very simple checks there. You may improve them in a future PR if you like, but at least they are covered now.

Thanks.

Note: I was waffling back and forth between how to refactor the input checks of the qplot_ functions (since they really should be identical to the requirements to the prep_ functions, just with default arguments). If you have a preference there, let me know.

Your implementation makes sense to me.

Note: Windows checks seem to be failing due to the breaking change to r2dii.data that introduced the new increasing_or_decreasing dataset.

This is strange because r2dii.plot is already using increasing_or_decreasing from r2dii.data. Unless something else changed.

[jdhoffa]

Indeed, it should be checking for presence of "projected" value in metric column. We missed that. The benchmark is optional. I propose to reword it in the documentation to:

• The column metric must have a "projected" value and
  a single scenario (e.g. "target_sds").
• (Optional) The column metric may contain a benchmark
  (e.g. "corporate_economy").

Awesome. Given that this PR is meant to only update checks (a maintenance update), and changing the docs would be user-facing, I would prefer to handle it in a separate PR. See new issue #557

And check if "projected" is in the metric column. The presence of scenario is checked in abort_if_multiple_scenarios(). To be consistent with what the function actually does we should maybe rename it to abort_if_wrong_number_of_scenarios() or something like that.

• I will look into this. Would you like me to also re-name the check? Is abort_if_wrong_number_of_scenarios() the name you prefer?

This is strange because r2dii.plot is already using increasing_or_decreasing from r2dii.data. Unless something else changed.

Indeed. I don't believe it is related to this PR, but I will look into it later. See #556

[MonikaFu]

Awesome. Given that this PR is meant to only update checks (a maintenance update), and changing the docs would be user-facing, I would prefer to handle it in a separate PR. See new issue #557
Sure

abort_if_wrong_number_of_scenarios() sounds good to me unless you have an idea for sth shorter.

[jdhoffa]

@MonikaFu this is ready for re-review

[MonikaFu]

LGTM