search

pik-piam / remind2

The remind2 package contains the REMIND-specific routines for data and model output manipulation.
0 stars 39 forks source link

REMIND reports negative fossil industry emissions #520

Open 0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q opened 5 months ago

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented 5 months ago 
> 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 commented 5 months ago

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

:eyes: 

$ git log --merges --after=2023-12-29 --before=2024-01-05
commit d30f3cc713fcdbd6b6d369f6c067282db8943481
Merge: bb3bd38 0c83272
Author: tabeado <117268634+tabeado@users.noreply.github.com>
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 <117268634+tabeado@users.noreply.github.com>
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 <tabea.dorndorf@pik-potsdam.de>
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 <69258269+fbenke-pik@users.noreply.github.com>
Date:   Thu Jan 4 13:34:05 2024 +0100

    Merge pull request #501 from fbenke-pik/master

    fix errors and warnings in reportEmi
0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented 5 months ago

501 is to blame https://github.com/pik-piam/remind2/commit/8593f4a0fc2a5728b3250de955f18a64f8499386#diff-0c2c1bfb6bf4b90bf16c9de64f770cf319e35c2683196b8bafbf1c04fd6b6ac4L2539

fbenke-pik commented 5 months ago

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: https://github.com/pik-piam/remind2/pull/489#discussion_r1440656520

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 commented 5 months ago

ok, I'll do that.

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 https://github.com/pik-piam/remind2/pull/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 commented 5 months ago

.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 commented 5 months ago

@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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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:

fbenke-pik commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 (https://github.com/pik-piam/remind2/pull/523). The other one (https://github.com/pik-piam/remind2/pull/489) is where I just forgot to run buildLibrary() and unleashed hell...

mellamoSimon commented 4 months ago

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 commented 4 months ago

(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.

fschreyer commented 4 months ago

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.