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

message_ix.Scenario.rename() does not work as expected #601

Closed
behnam-zakeri opened this issue May 6, 2022 · 3 comments · Fixed by #791
Closed

message_ix.Scenario.rename() does not work as expected #601

behnam-zakeri opened this issue May 6, 2022 · 3 comments · Fixed by #791
Assignees
Labels
bug Doesn't work as advertised/unintended effects

Comments

@behnam-zakeri
Copy link
Contributor

behnam-zakeri commented May 6, 2022

message_ix.Scenario.rename() is supposed to rename a set element, i.e., replacing the old name with a new name in all message_ix sets and parameters. However, this is not the case, i.e., not all sets and parameters are updated with a new name correctly, and there is no error or warning thrown stating this incompleteness. One can for example run the following code and check this.

It seems the issue is that under this method in message_ix.core.py, the check for finding a set element in a parameter is based on scenario.idx_names(). This means, e.g., when renaming a "node", the code only finds parameters that have index names of "node", but missing other relevant parameters that have index names related to node but not "node" itself, e.g., "node_loc", "node_rel" etc. This can be resolved relatively easily by using scenario.idx_sets() and with a little tweak of the code, and should be done possibly soon, as this is a very useful feature.

import ixmp as ix
import message_ix
mp = ix.Platform()
# Load a scenario and clone it
scen = message_ix.Scenario(mp, "Westeros_electrified", "baseline")
test = scen.clone(scenario="test", keep_solution=False)
# Rename an element from set "node" and keeping the original member
test.rename("node", {"Westeros": "Essos"}, keep=True)
# Check if the data exists in the original and renamed format
df = test.par("output", {"node_loc": "Westeros"})
assert not df.empty
df = test.par("output", {"node_loc": "Essos"})
assert not df.empty
@behnam-zakeri behnam-zakeri added the bug Doesn't work as advertised/unintended effects label May 6, 2022
@behnam-zakeri behnam-zakeri self-assigned this May 6, 2022
@behnam-zakeri
Copy link
Contributor Author

behnam-zakeri commented Feb 14, 2024

@khaeru and @glatterf42, following the demo in the TIGER workshop, I created this simple test related to this issue. Do you think this is good enough to be added to our test suite?

import pytest
from ixmp import Platform
from message_ix.testing import make_westeros


testdata = [
    ("technology", "coal_ppl", "coal_powerplant"),
    ("node", "Westeros", "Tessos"),
]


@pytest.mark.parametrize("set_name,old,new", testdata)
def test_rename(test_mp: Platform, set_name: str, old: str, new: str) -> None:
    """Test if the utility function `Scenario.rename()` works as expected.
    """
    scen_ref = make_westeros(test_mp, quiet=True)
    scen_ref.solve()

    # Clone to do renaming and tests
    scen = scen_ref.clone(keep_solution=False)

    # Rename old name to a new name
    scen.check_out()
    scen.add_set(set_name, new)
    scen.rename(set_name, {old: new})

    # Check if the new member has been added
    assert new in set(scen.set(set_name))

    # Check if the scenario solves and has a solution
    scen.solve()
    # Check if OBJ value is the same
    assert scen_ref.var("OBJ")["lvl"] == scen.var("OBJ")["lvl"]

@khaeru
Copy link
Member

khaeru commented Feb 14, 2024

Hi @behnam-zakeri —yes, that would be very welcome! Just to explain (for everyone) how this kind of simple test can be a valuable part of a bug fixing workflow, one could:

  1. Create a branch like issue/601.
  2. Commit the test with a message like "Add test of Scenario.rename, confirms message_ix.Scenario.rename() does not work as expected #601".
    • This makes a permanent relationship between the commit and this issue (we'll always be able to find the latter from the former).
    • The right place for this is probably in /tests.test_core.py. Our tests are not properly grouped yet, but one could even use this opportunity to set a test class that parallels the message_ix.Scenario class:
      class TestScenario:
          @pytest.mark.parametrize()
          def test_rename(self, test_mp, ...) -> None:
              ...
      (this would go near the top of the file: after the fixture but before any test_*() functions).
  3. Push the branch and open a PR with draft status.
  4. Make changes to the subject function/method to fix the bug.

If you don't have time to do these things, no problem—your comment is already very helpful and will make the work short for @glatterf42 or me. Please just tell us so.

behnam-zakeri added a commit to behnam-zakeri/message_ix that referenced this issue Feb 15, 2024
@behnam-zakeri
Copy link
Contributor Author

Thanks a lot @khaeru for the suggestions. I followed them and opened #791. I noticed the test class you mentioned is already there. So, I added the new test under the existing class.

@glatterf42 glatterf42 linked a pull request Feb 16, 2024 that will close this issue
3 tasks
glatterf42 added a commit that referenced this issue Feb 20, 2024
Improve `Scenario.rename()` to address #601
behnam-zakeri added a commit to behnam-zakeri/message_ix that referenced this issue Mar 12, 2024
behnam-zakeri added a commit that referenced this issue Mar 20, 2024
* Update multinode tutorial after resolving issue #601

* Extend multinode tutorial to include hints for questions

* Add release notes #798

* Apply ruff and format hint code block

* Rename tutorial to mention energy trade

* Update mypy, ruff, pre-commit config

Mypy v1.8.0 → v1.9.0
Ruff v0.3.0 → v0.3.2
pre-commit/action v3.0.0 → v3.0.1

---------

Co-authored-by: Fridolin Glatter <glatter@iiasa.ac.at>
Co-authored-by: Paul Natsuo Kishimoto <mail@paul.kishimoto.name>
glatterf42 added a commit to behnam-zakeri/message_ix that referenced this issue May 17, 2024
…a#798)

* Update multinode tutorial after resolving issue iiasa#601

* Extend multinode tutorial to include hints for questions

* Add release notes iiasa#798

* Apply ruff and format hint code block

* Rename tutorial to mention energy trade

* Update mypy, ruff, pre-commit config

Mypy v1.8.0 → v1.9.0
Ruff v0.3.0 → v0.3.2
pre-commit/action v3.0.0 → v3.0.1

---------

Co-authored-by: Fridolin Glatter <glatter@iiasa.ac.at>
Co-authored-by: Paul Natsuo Kishimoto <mail@paul.kishimoto.name>
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants