iiasa / message-ix-models

Tools for the MESSAGEix-GLOBIOM family of models
https://docs.messageix.org/models
Apache License 2.0
16 stars 31 forks source link

Raise informative exception from ScenarioInfo.io_units() #151

Closed khaeru closed 5 months ago

khaeru commented 5 months ago

Currently, if either the commodity or technology lacks units, the exception is internal to pint and could be hard to interpret:

File "/home/khaeru/vc/iiasa/models/message_ix_models/util/scenarioinfo.py", line 265, in io_units                                                                                                                                  
  return self.units_for("commodity", commodity) / self.units_for(                                                                                                                                                                  
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^                                                                                                                                                                
File "/home/khaeru/.venv/3.11/lib/python3.11/site-packages/pint/facets/plain/unit.py", line 159, in __truediv__
  return self._REGISTRY.Quantity(1 / other, self._units)
                                 ~~^~~~~~~
TypeError: unsupported operand type(s) for /: 'int' and 'NoneType'

How to review

Read the diff and note that the CI checks all pass.

PR checklist

codecov[bot] commented 5 months ago

Codecov Report

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

Comparison is base (99244af) 75.3% compared to head (ccd5637) 75.4%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #151 +/- ## ===================================== Coverage 75.3% 75.4% ===================================== Files 93 93 Lines 6204 6211 +7 ===================================== + Hits 4677 4684 +7 Misses 1527 1527 ``` | [Files](https://app.codecov.io/gh/iiasa/message-ix-models/pull/151?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iiasa) | Coverage Δ | | |---|---|---| | [message\_ix\_models/testing/\_\_init\_\_.py](https://app.codecov.io/gh/iiasa/message-ix-models/pull/151?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iiasa#diff-bWVzc2FnZV9peF9tb2RlbHMvdGVzdGluZy9fX2luaXRfXy5weQ==) | `100.0% <100.0%> (ø)` | | | [message\_ix\_models/tests/util/test\_scenarioinfo.py](https://app.codecov.io/gh/iiasa/message-ix-models/pull/151?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iiasa#diff-bWVzc2FnZV9peF9tb2RlbHMvdGVzdHMvdXRpbC90ZXN0X3NjZW5hcmlvaW5mby5weQ==) | `100.0% <100.0%> (ø)` | | | [message\_ix\_models/util/scenarioinfo.py](https://app.codecov.io/gh/iiasa/message-ix-models/pull/151?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iiasa#diff-bWVzc2FnZV9peF9tb2RlbHMvdXRpbC9zY2VuYXJpb2luZm8ucHk=) | `100.0% <100.0%> (ø)` | |
khaeru commented 5 months ago

FYI @glatterf42, I was able to debug this issue with a temporary commit that made a change like this:

@pytest.fixture(scope="function")
def test_context(request, session_context):
    """A copy of :func:`session_context` scoped to one test function."""
    from message_ix_models.model.config import Config

    ctx = deepcopy(session_context)

    # Defaults are preserved on session_context and thus ctx
    assert session_context.model.regions == Config.regions

    # Ensure there is a report key
    ctx.setdefault("report", dict())

    yield ctx

    ctx.delete()

    # Defaults are preserved on session_context and thus ctx
    assert session_context.model.regions == Config.regions

This helped me see that the assertions failed after tests which used the mix_models_cli fixture. This one was using the session_context fixture, and so changes made to that Context by various CLI commands would be copied within test_context and on to tests.

The last commit on this branch has the fix.