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

fix errors and warnings in reportEmi #501

Merged
merged 1 commit into from
Jan 4, 2024

Conversation

fbenke-pik
Copy link
Contributor

@fbenke-pik fbenke-pik commented Jan 4, 2024

This PR increments the library version, as it was not done in #489
In addition, warnings and errors caused by this PR in reportEmi are fixed as well.

@fbenke-pik fbenke-pik merged commit ff50a96 into pik-piam:master Jan 4, 2024
4 checks passed
@mellamoSimon
Copy link
Contributor

ups, too slow I guess... still, I think is good that @fschreyer takes a look. thank you for fixing my mess though, Falk! I thought the tests would fail if I didn't do that and I probably forgot to check manually. thanks again!

@@ -2536,10 +2535,9 @@ if (!is.null(vm_plasticsCarbon)) {

# 8. Ad-hoc fix for emissions w/o non-energy use and Aggregation to global and regional values ----

if (is.null(vm_demFENonEnergySector) && (module2realisation["industry", 2] == "fixed_shares")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, thanks. I guess, I overlooked that in the review as well. So far, these cases should still be covered by the reporting for backwards compatability also if industry realization is subsectors. We currently use this part for ARIADNE. Once we smoothly updated to the version including vm_demFENonEnergySector, I will remove these parts below.

# TODO: add non-energy use variables for all regionmappings and sector realizations
#Note (SM): I'm not sure if I got these notes so I created the condition above to try to make sure that this will work anyways

# Note: Non-energy use emissions should not be confused with process emissions. Non-energy use emissions are emissions/carbon flow of FE carriers which are used as feedstocks in industry.
if ("FE|Non-energy Use|Industry (EJ/yr)" %in% getNames(output) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

What would be important is that the version with vm_demFENonEnergySector does not enter this bracket. We do not need emissions variables corrected for non-energy use in Simon's implementation anymore. However, it looks like it does not enter here because Simon named the variables slightly differently e.g. FE|Non-energy Use|+|Industry (EJ/yr) instead of FE|Non-energy Use|Industry (EJ/yr). However, for clarity of the code it would advisable to add a is.null(vm_demFENonEnergySector) condition.

@fbenke-pik
Copy link
Contributor Author

This PR was just meant to make buildLibrary work again in the least intrusive way possible, as it was blocking others developing updates for remind2. Please address any follow-up issues in new PRs.

@mellamoSimon always remember to run lucode2::buildLibrary before merging your changes

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.

None yet

4 participants