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

use explicit gdx_refprices in convGDX2MIF, ignore LDV variables with NA unit #410

Merged
merged 3 commits into from
Jun 7, 2023

Conversation

orichters
Copy link
Contributor

@orichters orichters commented May 31, 2023

  • needs adaptation in remind such that the gdx_refprices is passed to convGDX2MIF.
  • I was a bit unsure whether to rename gdx_ref to gdx_refpolicycosts in order to be consistent, but I'm a bit afraid of breaking stuff… @LaviniaBaumstark / @Renato-Rodrigues, would you prefer that?
  • accept NA in some LDV units (see variables with unit NA #408), as suggested by @robertpietzcker
  • improve explanations in reportPrices, move transport aggregation before automated additions to avoid repetition, caused by adding fegat to fix reportPrices reports NA for fegat in transport #405
  • also ask to select file if colorScenConf is called not with "", but with NULL

Copy link
Contributor

@bs538 bs538 left a comment

Choose a reason for hiding this comment

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

Thanks Oliver!
If it can be done without breaking anything, I would be in favour of renaming, as it's easier to understand and more consistent with the scenario_config.csv (gdx_ref used for fixing in GAMS and prices, and gdx_ref_policycosts only for the policycosts)

@orichters orichters force-pushed the smallfix branch 2 times, most recently from 6a98a94 to e04aade Compare June 6, 2023 15:23
@orichters
Copy link
Contributor Author

Ok, so if gdx_refpolicycost is not specified, it defaults to gdx_ref, which allows to be backward-compatible. In REMIND, the situation should never happen that gdx_refpolicycost does not exist while gdx_ref is there because if path_gdx_refpolicycost is empty, it falls back automatically to path_gdx_ref.

@orichters
Copy link
Contributor Author

I now set gdx_refpolicycost = gdx_ref in the function description, so you can overwrite it as you like, but if it is not provided, it defaults to gdx_ref:

> z <- function(x = 3, y = x) return(paste("x:", x, "/ y:", y))
> z(3, 5)
[1] "x: 3 / y: 5"
> z(3)
[1] "x: 3 / y: 3"

@@ -5,26 +5,28 @@
#'
#'
#' @param gdx a GDX as created by readGDX, or the file name of a gdx
#' @param gdx_ref reference-gdx for policy costs, a GDX as created by readGDX, or the file name of a gdx
#' @param gdx_ref reference-gdx for < cm_startyear, used for fixing the prices to this scenario
Copy link
Member

Choose a reason for hiding this comment

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

there is no gdx_refargument anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that is not correct, we still need gdx_ref

#' @author Lavinia Baumstark
#' @examples
#'
#' \dontrun{convGDX2MIF(gdx,gdx_ref,file="REMIND_generic_default.csv",scenario="default")}
#' \dontrun{convGDX2MIF(gdx,gdx_refpolicycost,file="REMIND_generic_default.csv",scenario="default")}
#'
#' @export
#' @importFrom gdx readGDX
#' @importFrom magclass mbind write.report

convGDX2MIF <- function(gdx, gdx_ref = NULL, file = NULL, scenario = "default",
Copy link
Member

Choose a reason for hiding this comment

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

why do we need gdx_ref still for price correction?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because prices are Moving Averages, and if cm_startyear = 2025, and then the policy scenario calculates its 2020 price values, this takes into account the 2025 value which differs from its reference scenario, and so the reported prices would differ in 2020 even though they should not. See the long debates in #402

Copy link
Member

Choose a reason for hiding this comment

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

ah, thanks

@@ -691,15 +691,17 @@ reportPrices <- function(gdx, output=NULL, regionSubsetList=NULL,
out.reporting <- pmax(out, 0) # avoid negative prices

# for cm_startyear and non-SSP2, replace price by average of period before and after
# this is a workaround to avoid spikes caused by https://github.com/remindmodel/remind/issues/1068
Copy link
Member

Choose a reason for hiding this comment

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

but this it not used in default reporting in REMIND, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used for all non-SSP2 scenarios at the moment, as long as this remind issue is not fixed. Please discuss that with @soergel.

@bs538
Copy link
Contributor

bs538 commented Jun 7, 2023

Sounds good to me. I understand the user workflow for calculating policy costs is still the same after this, right?

  • selecting refpolicycost in scenario config OR manually through ex-post output.R -> policyCosts
  • (if rerunning reporting): automatically using right reference

@orichters
Copy link
Contributor Author

Sounds good to me. I understand the user workflow for calculating policy costs is still the same after this, right?

* selecting refpolicycost in scenario config OR manually through ex-post output.R -> policyCosts

* (if rerunning reporting): automatically using right reference

Yes, that remains unchanged. I thought about renaming the gdx_ref function parameter in reportPolicyCosts as well to gdx_ref_policycost, but then I thought it is unnecessary, may cause scripts to fail and the title of the function sufficiently clearly suggests that we are talking about policy costs here, I guess.

@orichters
Copy link
Contributor Author

For the record: Had a chat with @Renato-Rodrigues, he is fine with the changes in reportPrices.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reportPrices reports NA for fegat in transport
3 participants