Improve `Scenario.rename()` to address #601

behnam-zakeri commented 4 months ago

This PR improves the utility Scenario.rename() to resolve the bug identified in issue #601.

How to review

Copy the unit test introduced in this PR (see the file test_core.py to your message_ix main branch. The test should fail with the existing code.
Checkout to the branch of this PR, test should pass.

PR checklist

[x] Continuous integration checks all ✅
[x] New test is added ✅
[x] Update release notes.

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (a488b8a) 95.2% compared to head (9589d5a) 95.3%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #791 +/- ## ======================================= + Coverage 95.2% 95.3% +0.1% ======================================= Files 46 46 Lines 4335 4334 -1 ======================================= + Hits 4130 4134 +4 + Misses 205 200 -5 ``` | [Files](https://app.codecov.io/gh/iiasa/message_ix/pull/791?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iiasa) | Coverage Δ | | |---|---|---| | [message\_ix/core.py](https://app.codecov.io/gh/iiasa/message_ix/pull/791?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iiasa#diff-bWVzc2FnZV9peC9jb3JlLnB5) | `97.6% <100.0%> (+2.4%)` | :arrow_up: | | [message\_ix/tests/test\_core.py](https://app.codecov.io/gh/iiasa/message_ix/pull/791?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iiasa#diff-bWVzc2FnZV9peC90ZXN0cy90ZXN0X2NvcmUucHk=) | `100.0% <100.0%> (ø)` | |

behnam-zakeri commented 4 months ago

@glatterf42 the tests don't pass on python 3.9. Is this related to the way pytest.mark.parameterize() is unpacked between different versions of python?

glatterf42 commented 4 months ago

There are many different error messages which should not be there if some of the tests are passing at all like

FAILED message_ix/tests/test_core.py::TestScenario::test_rename[node-Westeros-Tessos-True] - assert 173795.09375 == (173795.09375 * 2)

or

FAILED message_ix/tests/test_core.py::TestScenario::test_rename[technology-coal_ppl-coal_powerplant-False] - RuntimeError: unhandled Java exception: The index set 'node' does not have an element 'Tessos'!

If these things are working for some tests, there should be no reason for them to suddenly fail for others. If tests fails but then suddenly succeed on a re-run, these tests are called flaky. Ideally, we want to get to the root cause of their flakiness and remove it. @khaeru has recently opened a PR with precisely that aim: https://github.com/iiasa/message_ix/pull/784 In that PR, the make_dantzig() fixture is adapted to be more robust. So I'm wondering whether we can fix these flaky tests by applying the exact same changes to make_westeros().

glatterf42 commented 4 months ago

make_westeros() already has an optional request argument, so I just passed a request from the fixture to it, let's see how this does.

BTW: another continent in the world of Game of Thrones is called Essos, which is tantalizingly close to the Tessos you used initially, so I took the freedom to rename it if you don't mind :)

glatterf42 commented 4 months ago

One fail remains, but that is one of our usual flaky suspects for which we haven't yet found the root cause, so I'll just re-run it and rest assured that all your tests are passing as expected :)

behnam-zakeri commented 4 months ago

Thanks @glatterf42 for reviewing this and the nice suggestion for the name Essos (I somehow mixed that). I see your point. It seems some subsequent tests loaded a scenario that was already manupulated by earlier tests. So, giving explicit scenario names using the "request" feature was very useful.

glatterf42 commented 4 months ago

One more minor suggestion (I think that's a typo), then we can merge this :)

khaeru commented 4 months ago

@behnam-zakeri @glatterf42 if it's alright, I will jump in here to make the added code a little more concise and probably performant.

khaeru commented 4 months ago

Done, see the first added commit for details.

I also expanded the test and documentation. Particularly:

I find that the method does not work for the "year" set, even though this is an index set. Or rather, it works, but the test fails because the resulting scenario is not feasible according to GAMS. I added a note to the docstring and a test case to confirm. I would suggest to make this a follow-up enhancement.
I added tests and docs for exceptions that come up if one calls the method with invalid arguments: name of an indexed set, name of a parameter, name of neither.
The method already adds the new elements to the named set. I think this was not clear from the docstring, because Behnam's new test called scen.add_set() unnecessarily. So added documentation to clearly state that the method does this.

behnam-zakeri commented 4 months ago

Thanks a lot, @khaeru. A learning class for avoiding for loops and if-statements, even when dealing with different item types (i.e., "par" and "set").

khaeru commented 4 months ago

@glatterf42 given the discussion in the comment thread above, I think this is probably ready to merge provided CI can pass.

glatterf42 commented 4 months ago

The CI seems fine, the errors are likely flaky, but I'll still confirm this before merging since we don't run the checks again on a merge anymore :)

iiasa / message_ix