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

read in corrected IEA_ETP notebooks #456

Merged
merged 6 commits into from
Feb 7, 2024
Merged

Conversation

fbenke-pik
Copy link
Contributor

No description provided.

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented Jan 11, 2024

Two points:

  • Like I said in the meeting, I would like to do a full comparison, so calibration + NPi + PkBudgs and compare scenarios plots. But there seems to be some madrat SNAFU preventing running REMIND with the data you generated, so we need to redo it.
  • Your changes would break old mrremind versions, as it requires to replace the files, which are not compatible.

I would therefore opt to add a parameter to readIEA_ETP() (and therefore calcIEA_ETP()) to explicitly select the version of the IEA ETPs to read, have it default to the "broken" version, and explicitly load the "new" version everywhere in mrremind. Ugly, but that is the structure we are cursed working with.

@fbenke-pik
Copy link
Contributor Author

preventing running REMIND with the data you generated, so we need to redo it.

According to Lavinia you can simply rename the generated files, if the additional hash in the names is the only problem.

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member

Sure. "Rename some files manually" is a cherished component of any reproducible science workflow …

@fbenke-pik
Copy link
Contributor Author

I was just proposing renaming the specific output file as a quick fix so we don't have to start inputdata generation again.

@fbenke-pik
Copy link
Contributor Author

I would therefore opt to add a parameter to readIEA_ETP() (and therefore calcIEA_ETP()) to explicitly select the version of the IEA ETPs to read, have it default to the "broken" version, and explicitly load the "new" version everywhere in mrremind. Ugly, but that is the structure we are cursed working with.

On it

@fbenke-pik fbenke-pik marked this pull request as ready for review January 12, 2024 12:54
@fbenke-pik
Copy link
Contributor Author

I would therefore opt to add a parameter to readIEA_ETP() (and therefore calcIEA_ETP()) to explicitly select the version of the IEA ETPs to read

Looks like readSource does not accept custom parameters (e.g. version). So if we wanted to make the version part of the input params, it would have to be via subtypes. This would blow up subtype values from 4 to 8 (4 subtypes x 2 version numbers).

Your changes would break old mrremind versions, as it requires to replace the files, which are not compatible.

If this is the only concern, there are other ways to ensure backwards compatibility. I adjusted the structure of the source file on the cluster so that older versions of mrremind read the faulty data and this takes the new data (v1.1)

Copy link
Member

Choose a reason for hiding this comment

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

I checked REMIND v3.2.1.dev902 with input data 6.68-A (dev) and 6.68-B (ETPfix) and the results match what was expected.
compScen-dev_ETPfix-2024-02-04_21.33.19-H12.pdf

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member

Here is the code used for the test, updated to origin/master. fbenke-pik#2

@fbenke-pik fbenke-pik merged commit a3f2939 into pik-piam:master Feb 7, 2024
2 checks passed
@fbenke-pik fbenke-pik deleted the iea_etp branch February 7, 2024 16:26
@fbenke-pik
Copy link
Contributor Author

Here is the code used for the test, updated to origin/master. fbenke-pik#2

Ah sorry, I missed that.

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.

2 participants