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.

Sign up for GitHub

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

Jump to bottom

REMIND reports negative fossil industry emissions #520

Closed
0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q opened this issue Feb 6, 2024 · 19 comments
Closed

REMIND reports negative fossil industry emissions #520

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q opened this issue Feb 6, 2024 · 19 comments
Assignees
@mellamoSimon
Labels
bug Something isn't working

Comments

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member 

> library(tidyverse)
> library(quitte)
> d <- read.quitte('/p/projects/remind/modeltests/remind/output/SSP2EU-NPi-AMT_2024-02-02_22.08.54/REMIND_generic_SSP2EU-NPi-AMT.mif')
> d %>% filter(grepl('^Emi\\|CO2\\|Energy\\|Demand\\|Industry\\|Chemicals\\|.*\\|Fossil$', variable), 0 > value) %>% df_variation() %>% arrange(period, region)
# A tibble: 98 × 4
   region variable                                                 period  value
   <fct>  <fct>                                                     <int>  <dbl>
 1 LAM    Emi|CO2|Energy|Demand|Industry|Chemicals|Solids|+|Fossil   2005 -1.69 
 2 OAS    Emi|CO2|Energy|Demand|Industry|Chemicals|Solids|+|Fossil   2005 -0.618
 3 EUR    Emi|CO2|Energy|Demand|Industry|Chemicals|Solids|+|Fossil   2015 -0.284
 4 CAZ    Emi|CO2|Energy|Demand|Industry|Chemicals|Solids|+|Fossil   2020 -3.33 
 5 LAM    Emi|CO2|Energy|Demand|Industry|Chemicals|Solids|+|Fossil   2020 -1.63 
 6 OAS    Emi|CO2|Energy|Demand|Industry|Chemicals|Solids|+|Fossil   2020 -1.29 
 7 SSA    Emi|CO2|Energy|Demand|Industry|Chemicals|Solids|+|Fossil   2020 -0.889
 8 USA    Emi|CO2|Energy|Demand|Industry|Chemicals|Solids|+|Fossil   2020 -3.37 
 9 CAZ    Emi|CO2|Energy|Demand|Industry|Chemicals|Solids|+|Fossil   2025 -3.28 
10 LAM    Emi|CO2|Energy|Demand|Industry|Chemicals|Solids|+|Fossil   2025 -2.18 
# ℹ 88 more rows
# ℹ Use `print(n = ...)` to see more rows
@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q 0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q added the bug Something isn't working label Feb 6, 2024
@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member Author

Since 5 January.

$ ls -rt SSP2EU-NPi-AMT_202*/REMIND_generic_SSP2EU-NPi-AMT.mif | tail | xargs grep -c 'Emi|CO2.*Fossil.*;-'
SSP2EU-NPi-AMT_2023-10-27_22.09.15/REMIND_generic_SSP2EU-NPi-AMT.mif:0
SSP2EU-NPi-AMT_2023-11-08_22.08.33/REMIND_generic_SSP2EU-NPi-AMT.mif:0
SSP2EU-NPi-AMT_2023-11-12_04.16.04/REMIND_generic_SSP2EU-NPi-AMT.mif:0
SSP2EU-NPi-AMT_2023-11-17_22.12.06/REMIND_generic_SSP2EU-NPi-AMT.mif:0
SSP2EU-NPi-AMT_2023-12-08_22.09.19/REMIND_generic_SSP2EU-NPi-AMT.mif:0
SSP2EU-NPi-AMT_2023-12-29_22.08.47/REMIND_generic_SSP2EU-NPi-AMT.mif:0
SSP2EU-NPi-AMT_2024-01-05_22.08.21/REMIND_generic_SSP2EU-NPi-AMT.mif:27
SSP2EU-NPi-AMT_2024-01-12_22.10.49/REMIND_generic_SSP2EU-NPi-AMT.mif:26
SSP2EU-NPi-AMT_2024-01-26_22.09.54/REMIND_generic_SSP2EU-NPi-AMT.mif:25
SSP2EU-NPi-AMT_2024-02-02_22.08.54/REMIND_generic_SSP2EU-NPi-AMT.mif:27

👀

$ git log --merges --after=2023-12-29 --before=2024-01-05
commit d30f3cc713fcdbd6b6d369f6c067282db8943481
Merge: bb3bd38 0c83272
Author: tabeado <[email protected]>
Date:   Thu Jan 4 16:02:07 2024 +0100

    Merge pull request #502 from tabeado/master
    
    bugfix dac and ew emissions

commit bb3bd383171892f1a897ab353e6c2c7a92ce5125
Merge: ff50a96 d875e21
Author: tabeado <[email protected]>
Date:   Thu Jan 4 14:45:33 2024 +0100

    Merge pull request #492 from tabeado/master
    
    adding average LCOccsinje and adjustment cost bugfixes

commit 2164f40fa5be7c02b09ae274550267f7528dc4b3
Merge: 78efc63 ff50a96
Author: tabeado <[email protected]>
Date:   Thu Jan 4 13:59:04 2024 +0100

    Merge branch 'master' of https://github.com/pik-piam/remind2

commit ff50a96dc4358f6bc29cb470074ee3f5f6a04ee7
Merge: 09b72be 8593f4a
Author: Falk Benke <[email protected]>
Date:   Thu Jan 4 13:34:05 2024 +0100

    Merge pull request #501 from fbenke-pik/master
    
    fix errors and warnings in reportEmi

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member Author

#501 is to blame 8593f4a#diff-0c2c1bfb6bf4b90bf16c9de64f770cf319e35c2683196b8bafbf1c04fd6b6ac4L2539

@fbenke-pik fbenke-pik removed their assignment Feb 6, 2024
@fbenke-pik
Copy link
Contributor

fbenke-pik commented Feb 6, 2024 •
edited
Loading

This was a fix to make sure that buildLibrary is working again so others could merge their work, as @mellamoSimon forgot to do it with his PR.

See also: #489 (comment)

Simón, please take a look and find a solution that also works with buildLibrary.

I guess the best way to move forward is bringing back the condition I had to remove and then running lucode2::buildLibrary() and addressing the issues that it yields.

@mellamoSimon
Copy link
Contributor

ok, I'll do that.

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member Author

This was a fix to make sure that buildLibrary is working again so others could merge their work, as @mellamoSimon forgot to do it with his PR.

See also: #489 (comment)

That was a pretty stupid fix.

@fbenke-pik
Copy link
Contributor

That was a pretty stupid fix.

What can I say. You were on vacation, so I guess smart fixes were not an option. Would have also helped I you had not approved the PR without buildLibrary in the first place.

But seriously, if you have nothing productive to contribute, better say nothing.

@mellamoSimon
Copy link
Contributor

hey guys,
I take full responsibility. I was in a rush and wasn't careful when merging the feedstocks reporting changes. I agree that now we should focus on fixing this. And we should also stay kind.
I prepared this draft PR #523 that re-introduces that condition. I didn't have to change anything else so I don't know why that was a problem before. Now I still need to fix the inconsistencies in emissions reporting...rn I'm testing if re-introducing this condition makes any difference. I will keep you updated.

@mellamoSimon
Copy link
Contributor

.rn I'm testing if re-introducing this condition makes any difference. I will keep you updated.

nope, it doesn't... which makes sense.

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member Author

@mellamoSimon
This https://github.com/0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q/remind2/tree/fix/520 makes buildLibrary() happy. Some variables are missing from the Ariadne reporting, but they were wrong anyhow.

@mellamoSimon
Copy link
Contributor

thank you! That looks reasonable. I was blindly trying to define the required sets to get it running but it would have probably just produced nonsense.
@fschreyer will need to have a look anyway.

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member Author

This was a fix to make sure that buildLibrary is working again so others could merge their work, as @mellamoSimon forgot to do it with his PR.

See also: #489 (comment)

What was bugging me: the Ariadne .gdx is totally fine with this code, is it used the subsectors realisation, not fixed_shares.

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member Author

That was a pretty stupid fix.

What can I say. You were on vacation, so I guess smart fixes were not an option. Would have also helped I you had not approved the PR without buildLibrary in the first place.

But seriously, if you have nothing productive to contribute, better say nothing.

That was uncalled for, and I would like to apologise for it.

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member Author

For the record: I didn't realize that the mapping tests can be skipped and buildLibrary still is successful when running locally (which is what had happened here)

[↑]

No, buildLibrary() did not run successfully on these changes, else it would have updated the package version. https://github.com/pik-piam/remind2/pull/489/files

Which tripped me up in the first place. The legit 0.128.0 is from #497, and the error is actually in #489, not #501.

So, to avoid a situation like this:

  • Could we have a workflow that rejects merges that do not update the version? I was under the impression that is the whole point behind the "ValidationKey" in .buildlibrary. A task for @fbenke-pik to bring up with RSE I guess.
  • I keep my stupid mouth shut, unless I have something helpful to say.

@fbenke-pik
Copy link
Contributor

That was a pretty stupid fix.

What can I say. You were on vacation, so I guess smart fixes were not an option. Would have also helped I you had not approved the PR without buildLibrary in the first place.
But seriously, if you have nothing productive to contribute, better say nothing.

That was uncalled for, and I would like to apologise for it.

Apology accepted :)

@fbenke-pik
Copy link
Contributor

fbenke-pik commented Feb 7, 2024 •
edited
Loading

No, buildLibrary() did not run successfully on these changes, else it would have updated the package version. https://github.com/pik-piam/remind2/pull/489/files

Correct.

I think what Simón meant is that all tests calling convGDX2MIF are silently skipped if you don't have the proper library setup:
skip_if_not(as.logical(gdxrrw::igdx(silent = TRUE)), "gdxrrw is not initialized properly")

So even if you run buildLibrary locally, you might skip them by accident.

This check exists because we haven't found a way to have the gdx libraries running in the Github workflows. It is all well, if you have the gdx libs installed locally and run buildLibrary. Otherwise, bugs can make it into the master branch. Not an optimal solution for sure.

@mellamoSimon
Copy link
Contributor

No, buildLibrary() did not run successfully on these changes, else it would have updated the package version

no no, I meant for the changes of that PR (#523). The other one (#489) is where I just forgot to run buildLibrary() and unleashed hell...

@mellamoSimon
Copy link
Contributor

Hi there,
so I think that eventually this problem can be traced back to remind. In particular, to the way we report energy demand at the subsectoral level with secondary energy carrier details. If I get it right from the code, we cannot really trace back subsectoral demand to the original secondary energy carrier and I think this issue is an artifact of the trick we use to do so.
So we basically assume that final energy (e.g., fesos) distributes homogenously over the possible secondary energy carriers (sesofos and sesobio) for a given subsector: we apply the same share o37_shIndFE to vm_demFeSector_afterTax for all possible secondary sources. The problem is that vm_demFENonEnergySector is now giving away this (dirty?) trick because it is subsector-specific (basically, it is defined for chemicals only). Then, for example, if in a given region for a given timestep, like the ones in the example above, it turns out that all fesos used as feedstock comes from sesofos (which in this case it is actually a result of the variables that participate in the optimization problem) and none from sesobio, the postprocessing will still allocate a part of this energy to sesobio when calculating total feso demand in the chemicals subsector (o37_demFeIndSub). This results in the reporting trick assigning a lower value o37_demFeIndSub(sesofos,feso) than it actually has or can have. Later, when this variable gets passed on to remind2, we calculate energy flows without feedstocks (o37_demFeIndSub_woNonEn) here and that variable will take negative values because the trick-calculated total FE demand (o37_demFeIndSub(sesofos,feso)) is too low. Then, when we multiply a negative energy flow by the emission factor, we get these negative emissions.
This is just my working hypothesis at the moment, please feel free to refute it. If this is true, I see 2 possible solutions: (1) we get rid of those reported variables because they are anyway artificial or (2) in remind's post-processing, we separate the calculations for the chemicals subsector and allocate secondary sources independently for energy- and feedstock-use of energy carriers.

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member Author

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented Feb 9, 2024 •
edited
Loading

(1) we get rid of those reported variables because they are anyway artificial

Not an option. These variables are required to disaggregate what is going on in industry.

And I disagree with these variables being artificial. We might go somewhat roundabout in calculating them, but they are meaningful.

Closed
@fschreyer
Copy link
Contributor

Hey Simon,

thanks for looking into that. It makes sense to me. it would also go for option 2). As you say, I think that could be done by restricting the calculation of FE per industry subsector in the postsolve.gms to FE excluding non-energy use. So, the non-energy use FE should be subtracted in the calculations of o37_demFeIndTotEn, o37_shIndFE and o37_demFeIndSub if I see correctly. Then, o37_shIndFE would only be the share of a sector in total FE for energy that is actually combusted. Afterwards, these two lines in the emissions reporting, I believe, would need to be deleted as o37_demFeIndSub will already be w/o non-energy use (maybe even rename the parameter for clarity). Hope I don't overlook something, but that would be my idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mellamoSimon mellamoSimon

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@fschreyer @0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q @fbenke-pik @mellamoSimon