Improve Scenario.rename() to address #601

[behnam-zakeri]

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

• Continuous integration checks all ✅
• New test is added ✅
• Update release notes.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

@@           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	Coverage Δ
message_ix/core.py	97.6% <100.0%> (+2.4%)	⬆️
message_ix/tests/test_core.py	100.0% <100.0%> (ø)

[behnam-zakeri]

@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]

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: #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]

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]

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 :)

[glatterf42]

Looks good to me, thanks :)

[behnam-zakeri]

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]

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

[khaeru]

@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]

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]

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]

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

[glatterf42]

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 :)