Resolve the issue of zero capacity factor #561

behnam-zakeri · 2022-02-16T10:54:21Z

This PR adds tests for parameter "capacity_factor" to address the requirements mentioned in #515.

How to review

Please make a copy of this branch, run the test with the existing code, and the tests should pass.

PR checklist

Reduce duplication, i.e. don't have identical model_generator() methods in test_feature_temporal_level.py and test_feature_capacity_factor.py (@khaeru)
Continuous integration checks all ✅
Add or expand tests; coverage checks both ✅
Release notes added.

codecov · 2022-02-16T11:10:57Z

Codecov Report

Merging #561 (53641ea) into main (27bdc65) will increase coverage by 0.1%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##            main    #561     +/-   ##
=======================================
+ Coverage   92.7%   92.8%   +0.1%     
=======================================
  Files         41      42      +1     
  Lines       3236    3287     +51     
=======================================
+ Hits        3001    3052     +51     
  Misses       235     235

Impacted Files	Coverage Δ
message_ix/message_ix/models.py	`100.0% <0.0%> (ø)`
...ix/message_ix/tests/test_feature_temporal_level.py	`100.0% <0.0%> (ø)`
...x/message_ix/tests/test_feature_capacity_factor.py	`100.0% <0.0%> (ø)`
message_ix/message_ix/testing/__init__.py	`99.6% <0.0%> (+<0.1%)`	⬆️

behnam-zakeri · 2022-02-16T15:59:28Z

@khaeru, I went further and resolved the issue in the GAMS formulation, by adding a new masking set ("is_capacity_factor"). For populating this set on the python side, I copied the code you suggested in #514 (the part related to the enforce() method). I wanted to make sure that the tests designed here will work if we correct the formulation. Now, you can decide if we should add enforce() workflow here, or in a separate PR.

khaeru · 2022-02-21T14:52:18Z

Will jump in here once #559 is addressed.

khaeru · 2022-04-25T11:26:54Z

Reduce duplication, i.e. don't have identical model_generator() methods in test_feature_temporal_level.py and test_feature_capacity_factor.py (@khaeru)

@behnam-zakeri I've now done this (about 2.5 hours), saving us about 400 lines of code in the test files while still doing all the same tests. To me, at least, they are now easier to read & distinguish from one another. However, one question came up in the process, so I hope you can help:

Two of the tests (test_two_seasons_to_year_relative and test_linked_three_temporal_levels_relative) had the following pattern of code:

def test_func():
    try:
        do_stuff()
    except AssertionError:
        pass

This test will pass if do_stuff() raises an AssertionError. But it will also pass if do_stuff() doesn't raise any error at all. So the expected behaviour is ambiguous.

I changed these to the standard way of using pytest:

def test_func():
    with pytest.raises(AssertionError):
        do_stuff()

This is more explicit: it will pass if do_stuff() raises an AssertionError as expected, but the test will fail if the error is not raised.

I find these tests now fail. From the docstrings/comments:

test_two_seasons_to_year_relative says "Model should not solve," meaning we expect some kind of failure?
test_linked_three_temporal_levels_relative says "Model solves" in the docstring but, in contrast, "Shouldn't pass with a relative time duration" in the comments.

Can you identify what's going on in each case? What do we actually expect—maybe that the model actually should solve in each case, but the assertions in check_solution(scen) should fail?

khaeru · 2022-04-25T11:30:08Z

maybe that the model actually should solve in each case, but the assertions in check_solution(scen) should fail?

The last commit makes this change, and the tests now pass, so I assume that was the case.

behnam-zakeri · 2022-04-25T20:22:16Z

Thanks a lot @khaeru for reformatting and improvements. Related to your question
“ … that the model actually should solve in each case, but the assertions in check_solution(scen) should fail? that the model actually should solve in each case, but the assertions in check_solution(scen) should fail?”
As you identified in these cases the model should solve but Assersion checks must not pass. These checks ensure commodity balances wok as expected across different temporal levels.

- Adapted from .test_feature_capacity_factor.model_generator() with the following improvements: - Don't generate and assert in the same function; split assertions to a separate function, check_solution() that remains with test code. - Use the pytest `request` fixture to access `test_mp` instead of passing it separately. - Use `request.node.name` instead of defining and passing a `comment` argument. - Remove the `year` argument, since calls to the function never override the default. - Collapse separate function add_cap_par() into the function body, since this is the only place it is used. - Use make_df() instead of assuming dimension order. - Use PEP 257-compliant docstrings, i.e. initial line is a single line.

Saves ~100 LOC

Improve docstrings

LauWien

Looks good, thank you! Just some, suggestions in line.

LauWien · 2022-04-26T07:36:52Z

message_ix/models.py

+            if existing.equals(expected):
+                continue  # Contents are as expected; do nothing
+            # else:
+            #     scenario.add_set(set_name, expected)


Just to be sure, both lines above are commented.

Yes, I'm not sure why; they're not on #514 from which this was cherry-picked. That PR will need to be deconflicted when it is rebased, so we can clean up/remove then.

message_ix/tests/test_feature_temporal_level.py

message_ix/tests/test_feature_capacity_factor.py

LauWien · 2022-04-26T08:07:12Z

message_ix/tests/test_feature_temporal_level.py

+    tec_dict["gas_ppl"]["time_origin"] = ["year"]
+
+    scen = make_subannual(
+        request,
        tec_dict,
        time_steps=[("summer", 1, "season", "year")],


We could also think about moving this up and create e.g. TS_0 to reduce duplication (this line and line 75).

Ah, true. I skipped this one because it doesn't save on # of lines.

message_ix/tests/test_feature_capacity_factor.py

Co-authored-by: Laura Wienpahl <57132039+LauWien@users.noreply.github.com>

khaeru · 2022-04-26T09:57:16Z

Thanks for the review! Will merge once checks again pass.

khaeru · 2022-04-26T10:38:04Z

Thanks @behnam-zakeri for the contributions! 🎉

behnam-zakeri mentioned this pull request Feb 16, 2022

Zero values for capacity_factor are changed to 1 in GAMS #515

Closed

khaeru assigned khaeru and behnam-zakeri Feb 16, 2022

khaeru linked an issue Feb 16, 2022 that may be closed by this pull request

Zero values for capacity_factor are changed to 1 in GAMS #515

Closed

behnam-zakeri changed the title ~~Add test for capacity factor~~ Resolve the issue of zero capacity factor Feb 16, 2022

khaeru force-pushed the test-capacity-factor branch from eb27371 to 5f25a49 Compare April 25, 2022 09:05

khaeru pushed a commit to behnam-zakeri/message_ix that referenced this pull request Apr 25, 2022

Add iiasa#561 to release notes

1a4182e

khaeru force-pushed the test-capacity-factor branch from e697161 to 11a503b Compare April 25, 2022 11:15

khaeru requested a review from a team April 25, 2022 11:31

khaeru pushed a commit to behnam-zakeri/message_ix that referenced this pull request Apr 25, 2022

Add iiasa#561 to release notes

5309605

khaeru force-pushed the test-capacity-factor branch from 87dff44 to bf145f9 Compare April 25, 2022 11:34

khaeru added this to the 3.5 milestone Apr 25, 2022

LauWien requested review from LauWien and removed request for a team April 26, 2022 06:31

behnam-zakeri and others added 10 commits April 26, 2022 09:22

Add test for capacity factor

be055a8

Apply black for code style

6384afd

Copy enforcing new mapping set from PR#514

7dae317

Add the masking set on the python side

0fc16a1

Add masking set (is_capacity_factor) in GAMS

620982d

Improve tests to work with the new formulation

e153ac7

Add iiasa#561 to release notes

3c0e68c

Update technology names for more intuition

148018a

Use make_subannual() to handle test_feature_temporal_level cases

de42cc6

khaeru added 5 commits April 26, 2022 09:22

Use globals instead of duplicates in test_feature_temporal_level

1ddb2f1

Saves ~100 LOC

Simplify make_subannual()

c64b6d8

Use globals instead of duplicates in test_feature_capacity_factor

ff7ca20

Improve docstrings

Adjust expected failures in two tests of temporal levels

eff15d1

Adjust reporting tests for new is_capacity_factor item

fae1455

khaeru force-pushed the test-capacity-factor branch from 1dec9a5 to fae1455 Compare April 26, 2022 07:22

LauWien approved these changes Apr 26, 2022

View reviewed changes

Apply suggestions from code review

53641ea

Co-authored-by: Laura Wienpahl <57132039+LauWien@users.noreply.github.com>

khaeru mentioned this pull request Apr 26, 2022

Assignment of capacity_factor is not properly working for subannual time slices #409

Closed

khaeru merged commit e7a525c into iiasa:main Apr 26, 2022

LauWien mentioned this pull request Apr 27, 2022

Address failing nightly workflow #591

Closed

khaeru mentioned this pull request Apr 28, 2022

Techs incorrectly excluded from capacity_factor via map_tec_time #592

Open

LauWien pushed a commit to LauWien/message_ix that referenced this pull request Apr 28, 2022

Add iiasa#561 to release notes

4052f2e

khaeru pushed a commit that referenced this pull request May 6, 2022

Explain #561, #591 in migration notes

b5a8aa7

This was referenced May 25, 2022

Ensure MESSAGE.enforce() runs for MESSAGE_MACRO #613

Merged

Adjust typing for mypy 0.960 & other minor fixes iiasa/ixmp#450

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve the issue of zero capacity factor #561

Resolve the issue of zero capacity factor #561

behnam-zakeri commented Feb 16, 2022 •

edited by khaeru

Loading

codecov bot commented Feb 16, 2022 •

edited

Loading

behnam-zakeri commented Feb 16, 2022

khaeru commented Feb 21, 2022

khaeru commented Apr 25, 2022 •

edited

Loading

khaeru commented Apr 25, 2022

behnam-zakeri commented Apr 25, 2022

LauWien left a comment

LauWien Apr 26, 2022

khaeru Apr 26, 2022

LauWien Apr 26, 2022

khaeru Apr 26, 2022

khaeru commented Apr 26, 2022

khaeru commented Apr 26, 2022

Resolve the issue of zero capacity factor #561

Resolve the issue of zero capacity factor #561

Conversation

behnam-zakeri commented Feb 16, 2022 • edited by khaeru Loading

How to review

PR checklist

codecov bot commented Feb 16, 2022 • edited Loading

Codecov Report

behnam-zakeri commented Feb 16, 2022

khaeru commented Feb 21, 2022

khaeru commented Apr 25, 2022 • edited Loading

khaeru commented Apr 25, 2022

behnam-zakeri commented Apr 25, 2022

LauWien left a comment

Choose a reason for hiding this comment

LauWien Apr 26, 2022

Choose a reason for hiding this comment

khaeru Apr 26, 2022

Choose a reason for hiding this comment

LauWien Apr 26, 2022

Choose a reason for hiding this comment

khaeru Apr 26, 2022

Choose a reason for hiding this comment

khaeru commented Apr 26, 2022

khaeru commented Apr 26, 2022

behnam-zakeri commented Feb 16, 2022 •

edited by khaeru

Loading

codecov bot commented Feb 16, 2022 •

edited

Loading

khaeru commented Apr 25, 2022 •

edited

Loading