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

Correct GAMS for the assignment of "capacity_factor" at "year" #705

Merged
merged 6 commits into from
Apr 18, 2023

Conversation

behnam-zakeri
Copy link
Contributor

@behnam-zakeri behnam-zakeri commented Apr 17, 2023

This PR resolves issue #409. The GAMS formulation is corrected not to assign a capacity factor of 1 at "year" if the value is missing in a scenario with sub-annual time slices. The weighted average of capacity factors defined for sub-annual time slices can be used to calculate the capacity factor for "year". This is tested by extending the existing tests in test_capacity_factor_feature, i.e., adding data for parameter "operation_factor", which uses capacity factor at "year".

How to review

The test test_capacity_factor_feature should pass with the GAMS formulation in this PR. However, this test must fail if using GAMS formulation of the main branch of message_ix. So, first, copy this test to your main branch and ensure it fails. Then, switch to this branch and run the tests again and make sure they pass.

PR checklist

  • Add or expand tests; coverage checks both ✅
  • [Not needed ] Add, expand, or update documentation.
  • Update release notes.

behnam-zakeri added a commit to behnam-zakeri/message_ix that referenced this pull request Apr 17, 2023
@behnam-zakeri behnam-zakeri added bug Doesn't work as advertised/unintended effects timeslice labels Apr 17, 2023
@codecov
Copy link

codecov bot commented Apr 17, 2023

Codecov Report

Merging #705 (55abd89) into main (a824be9) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@          Coverage Diff          @@
##            main    #705   +/-   ##
=====================================
  Coverage   94.4%   94.4%           
=====================================
  Files         43      43           
  Lines       3432    3449   +17     
=====================================
+ Hits        3241    3258   +17     
  Misses       191     191           
Impacted Files Coverage Δ
message_ix/testing/__init__.py 99.6% <100.0%> (+<0.1%) ⬆️
message_ix/tests/test_feature_capacity_factor.py 100.0% <100.0%> (ø)

@glatterf42
Copy link
Member

Can confirm: on my main branch, the new test fails with the following error message: FAILED message_ix/tests/test_feature_capacity_factor.py::test_capacity_factor_average - assert 3.0 == 2.8
At the same time, the test succeeds on the new branch.

While this looks good, I'm also receiving a new RuntimeWarning:

message_ix/tests/test_feature_capacity_factor.py::test_capacity_factor_zero_two
home/glatterf42/message_ix/message_ix/tests/test_feature_capacity_factor.py:27: RuntimeWarning: invalid value encountered in double_scalars
    act.loc[i, "cf-corrected"] = act.loc[i, "lvl"] / float(

I will try to understand where that comes from.

@glatterf42 glatterf42 force-pushed the capacity-factor-average branch from 6ae50f5 to a0dc951 Compare April 18, 2023 07:17
glatterf42 pushed a commit to behnam-zakeri/message_ix that referenced this pull request Apr 18, 2023
Copy link
Member

@glatterf42 glatterf42 left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I'd like to understand this RuntimeWarning before approving.

)
if i[1] != "year":
cf.loc[i, "duration-corrected"] = cf.loc[i, "value"] * duration
act.loc[i, "cf-corrected"] = act.loc[i, "lvl"] / float(
Copy link
Member

Choose a reason for hiding this comment

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

This seems to cause the RuntimeWarning. I'm wondering about i and the hardcoded index 1: is that always correct or does it assume a certain structure of i? If so, can we change to using no assumption?

Copy link
Member

Choose a reason for hiding this comment

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

The problem arises in the test_capacity_factor_zero_two, where the solar_pv_ppl has a capacity_factor of 0 in the time slice night. In

if i[1] != "year":
            cf.loc[i, "duration-corrected"] = cf.loc[i, "value"] * duration
            act.loc[i, "cf-corrected"] = act.loc[i, "lvl"] / float(
                cf.loc[i, "duration-corrected"]
            )

this gives us a division by 0 since cf.loc[i, "value"] is 0. So we have to provide some condition/prescription on how to deal with cases like this.

Copy link
Member

Choose a reason for hiding this comment

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

I've now added a condition that checks if either duration or cf.loc[i, "value"] are 0. In that case, we now set act.loc[i, "cf-corrected"] to act.loc[i, "lvl"], so we don't change it. This relies on the assumption that in this case, act.loc[i, "lvl"] will also be 0, which is implied (and asserted) by the test in line 19.
All the tests are still passing for me, but there's no RuntimeWarning 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.

Thanks @glatterf42 for reviewing this and suggesting improvements. Indeed, as you noted, zero capacity factors are tested and they can be excluded from the follow-up calculations. The index i in that loop is a tuple with two members, the first one being the technology name and the second time slices. So, the index 1 is always related to time.

@glatterf42 glatterf42 force-pushed the capacity-factor-average branch from a0dc951 to 09fca14 Compare April 18, 2023 09:35
@glatterf42 glatterf42 force-pushed the capacity-factor-average branch from 09fca14 to 55abd89 Compare April 18, 2023 09:39
@glatterf42 glatterf42 merged commit 1955e04 into iiasa:main Apr 18, 2023
@khaeru khaeru added this to the 3.7 milestone Apr 19, 2023
ywpratama pushed a commit to ywpratama/message_ix that referenced this pull request Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Doesn't work as advertised/unintended effects timeslice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assignment of capacity_factor is not properly working for subannual time slices
3 participants