From 5c289c6d8b7ea53b29beffac5991586d90f9e0ec Mon Sep 17 00:00:00 2001 From: Paul Natsuo Kishimoto Date: Tue, 26 Jul 2022 17:42:57 +0200 Subject: [PATCH 1/8] Use check_out(timeseries_only=True) in store_ts() --- ixmp/reporting/computations.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ixmp/reporting/computations.py b/ixmp/reporting/computations.py index 2aeb9121e..25e3980aa 100644 --- a/ixmp/reporting/computations.py +++ b/ixmp/reporting/computations.py @@ -187,7 +187,7 @@ def store_ts(scenario, *data): import pyam log.info(f"Store time series data on '{scenario.url}'") - scenario.check_out() + scenario.check_out(timeseries_only=True) for order, df in enumerate(data): df = ( From 8f6ae3bd42c1cc2f32d68e3005aa6d4b03ad8250 Mon Sep 17 00:00:00 2001 From: Paul Natsuo Kishimoto Date: Wed, 27 Jul 2022 11:31:19 +0200 Subject: [PATCH 2/8] Avoid squashing config settings with downstream defaults --- ixmp/_config.py | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/ixmp/_config.py b/ixmp/_config.py index e75237af0..e55551032 100644 --- a/ixmp/_config.py +++ b/ixmp/_config.py @@ -82,25 +82,24 @@ def __getitem__(self, name): def __setitem__(self, name, value): setattr(self, name.replace(" ", "_"), value) - def add_field(self, name, type_, default=None): + def add_field(self, name, type_, default, **kwargs): # Check `name` name = name.replace(" ", "_") - if name in self.__dataclass_fields__: + if ( + name in self.__dataclass_fields__ + and "auto" not in self.__dataclass_fields__[name].metadata + ): raise ValueError(f"configuration key {repr(name)} already defined") # Create a new data class with an additional field new_cls = make_dataclass( - "Values", [(name, type_, field(default=default))], bases=(self.__class__,) + "Values", + [(name, type_, field(default=default, **kwargs))], + bases=(self.__class__,), ) # Re-use current values and any defaults for the new fields - data = asdict(self) - try: - data[name] = getattr(self, name) - except AttributeError: - pass - - return new_cls, new_cls(**data) + return new_cls, new_cls(**asdict(self)) def delete_field(self, name): # Check `name` @@ -249,9 +248,11 @@ def read(self): # Parse JSON and set values for key, value in data.items(): try: - self.set(key, value, _strict=True) # Cast type for registered keys + self.set(key, value) # Cast type for registered keys except KeyError: - self.set(key, value, _strict=False) # Tolerate unregistered keys + # Automatically register new values + self.register(key, type(value), default=None, metadata=dict(auto=True)) + self.set(key, value) # Public methods @@ -263,7 +264,7 @@ def keys(self) -> Tuple[str, ...]: """Return the names of all registered configuration keys.""" return self.values.keys() - def register(self, name: str, type_: type, default: Any = None): + def register(self, name: str, type_: type, default: Any = None, **kwargs): """Register a new configuration key. Parameters @@ -281,7 +282,9 @@ def register(self, name: str, type_: type, default: Any = None): ValueError if the key `name` is already registered. """ - self._ValuesClass, self.values = self.values.add_field(name, type_, default) + self._ValuesClass, self.values = self.values.add_field( + name, type_, default, **kwargs + ) def unregister(self, name: str) -> None: """Unregister and clear the configuration key `name`.""" From 0eba270ad3a91249f7e53b57024bd6debc83775a Mon Sep 17 00:00:00 2001 From: Paul Natsuo Kishimoto Date: Wed, 27 Jul 2022 11:32:36 +0200 Subject: [PATCH 3/8] Handle exception in JDBCBackend.set_data() Partly addresses #296 --- ixmp/backend/jdbc.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/ixmp/backend/jdbc.py b/ixmp/backend/jdbc.py index 71408aa23..ea62f9946 100644 --- a/ixmp/backend/jdbc.py +++ b/ixmp/backend/jdbc.py @@ -805,7 +805,16 @@ def set_data(self, ts, region, variable, data, unit, subannual, meta): # Integer so JPype does not produce invalid java.lang.Long. jdata = java.LinkedHashMap({java.Integer(k): v for k, v in data.items()}) - self.jindex[ts].addTimeseries(region, variable, subannual, jdata, unit, meta) + try: + self.jindex[ts].addTimeseries( + region, variable, subannual, jdata, unit, meta + ) + except java.IxException as e: + match = re.search("node '([^']*)' does not exist in the database", str(e)) + if match: + raise ValueError(f"region = {match.group(1)}") from None + else: + raise def set_geo(self, ts, region, variable, subannual, year, value, unit, meta): self.jindex[ts].addGeoData( From dfc5523ad82beaf0c1c7f9e351dcd8403f542e1c Mon Sep 17 00:00:00 2001 From: Paul Natsuo Kishimoto Date: Wed, 27 Jul 2022 11:33:18 +0200 Subject: [PATCH 4/8] Streamline units handling in .computations.update_scenario() --- ixmp/reporting/computations.py | 9 ++++++--- ixmp/tests/reporting/test_computations.py | 10 +++++++++- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/ixmp/reporting/computations.py b/ixmp/reporting/computations.py index 25e3980aa..2a542ba76 100644 --- a/ixmp/reporting/computations.py +++ b/ixmp/reporting/computations.py @@ -222,9 +222,12 @@ def update_scenario(scenario, *quantities, params=[]): if not isinstance(qty, pd.DataFrame): # Convert a Quantity to a DataFrame par_name = qty.name - new = qty.to_series().reset_index().rename(columns={par_name: "value"}) - new["unit"] = "{:~}".format(qty.attrs["_unit"]) # type: ignore [str-format] - qty = new + qty = ( + qty.to_series() + .reset_index() + .rename(columns={par_name: "value"}) + .assign(unit=f"{qty.units:~}") + ) # Add the data log.info(f" {repr(par_name)} ← {len(qty)} rows") diff --git a/ixmp/tests/reporting/test_computations.py b/ixmp/tests/reporting/test_computations.py index 0176665c1..ece9ac50a 100644 --- a/ixmp/tests/reporting/test_computations.py +++ b/ixmp/tests/reporting/test_computations.py @@ -4,7 +4,7 @@ import pandas as pd import pyam import pytest -from genno import Computer, Quantity +from genno import ComputationError, Computer, Quantity from genno.testing import assert_qty_equal from pandas.testing import assert_frame_equal @@ -135,3 +135,11 @@ def test_store_ts(request, caplog, test_mp): # Input is stored exactly assert_frame_equal(expected_1, scen.timeseries(variable="Foo")) assert_frame_equal(expected_2, scen.timeseries(variable="Bar")) + + # Data with an unregistered region name + c.add("input 3", test_data[0].assign(variable="Foo", region="Moon")) + c.add("test 2", store_ts, "target", "input 3") + + # The computation fails + with pytest.raises(ComputationError, match="baz"): + c.get("test 2") From 7081be3a46e620d0a750921b2ddb1e15b5a122f2 Mon Sep 17 00:00:00 2001 From: Paul Natsuo Kishimoto Date: Fri, 12 Aug 2022 18:37:43 +0200 Subject: [PATCH 5/8] Use genno.Computer.require_compat() from 1.12 --- ixmp/reporting/reporter.py | 9 +-------- setup.cfg | 2 +- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/ixmp/reporting/reporter.py b/ixmp/reporting/reporter.py index 263081dd4..4b8406558 100644 --- a/ixmp/reporting/reporter.py +++ b/ixmp/reporting/reporter.py @@ -17,14 +17,7 @@ def __init__(self, *args, **kwargs): # Append ixmp.reporting.computations to the modules in which the Computer will # look up computations names. - # genno <= 1.11 - from ixmp.reporting import computations - - if computations not in self.modules: - self.modules.append(computations) - - # TODO use this once genno >= 1.12.0 is released - # self.require_compat("ixmp.reporting.computations") + self.require_compat("ixmp.reporting.computations") @classmethod def from_scenario(cls, scenario: Scenario, **kwargs) -> "Reporter": diff --git a/setup.cfg b/setup.cfg index 7e1ffc46a..c1b6e1600 100644 --- a/setup.cfg +++ b/setup.cfg @@ -30,7 +30,7 @@ zip_safe = True include_package_data = True install_requires = click - genno >= 1.11.0 + genno >= 1.12.0 JPype1 >= 1.2.1 openpyxl pandas >= 1.2 From ed034e66badb2c0b0e6ca212dad9c12416e7837e Mon Sep 17 00:00:00 2001 From: Paul Natsuo Kishimoto Date: Fri, 12 Aug 2022 19:49:24 +0200 Subject: [PATCH 6/8] Optionally tolerate failures in .reporting.computations.store_ts() --- ixmp/reporting/computations.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/ixmp/reporting/computations.py b/ixmp/reporting/computations.py index 2a542ba76..d43dc7660 100644 --- a/ixmp/reporting/computations.py +++ b/ixmp/reporting/computations.py @@ -167,7 +167,7 @@ def map_as_qty(set_df: pd.DataFrame, full_set): ) -def store_ts(scenario, *data): +def store_ts(scenario, *data, strict: bool = False) -> None: """Store time series `data` on `scenario`. The data is stored using :meth:`.add_timeseries`; `scenario` is checked out and then @@ -180,10 +180,11 @@ def store_ts(scenario, *data): data : pandas.DataFrame or pyam.IamDataFrame 1 or more objects containing data to store. If :class:`pandas.DataFrame`, the data are passed through :func:`to_iamc_layout`. + strict: bool + If :data:`True` (default :data:`False`), raise an exception if any of `data` are + not successfully added. Otherwise, log on level :ref:`ERROR ` and + continue. """ - # TODO tolerate invalid types/errors on elements of `data`, logging exceptions on - # level ERROR, then continue and commit anyway; add an optional parameter like - # continue_on_error=True to control this behaviour import pyam log.info(f"Store time series data on '{scenario.url}'") @@ -197,8 +198,14 @@ def store_ts(scenario, *data): ) # Add the data - log.info(f" ← {len(df)} rows") - scenario.add_timeseries(df) + try: + scenario.add_timeseries(df) + except Exception as e: + log.error(f"Failed with {e!r}:\n{df}") + if strict: + raise + else: + log.info(f" ← {len(df)} rows") scenario.commit(f"Data added using {__name__}") From c5c89df165f8d893d0cfb6bd5eba792fb0355de7 Mon Sep 17 00:00:00 2001 From: Paul Natsuo Kishimoto Date: Fri, 12 Aug 2022 19:50:38 +0200 Subject: [PATCH 7/8] Adjust reporting tests --- ixmp/tests/reporting/test_computations.py | 23 +++++++++++++++++++++-- ixmp/tests/reporting/test_reporter.py | 4 +--- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/ixmp/tests/reporting/test_computations.py b/ixmp/tests/reporting/test_computations.py index ece9ac50a..20ecb4d8e 100644 --- a/ixmp/tests/reporting/test_computations.py +++ b/ixmp/tests/reporting/test_computations.py @@ -1,4 +1,5 @@ import logging +import re from functools import partial import pandas as pd @@ -140,6 +141,24 @@ def test_store_ts(request, caplog, test_mp): c.add("input 3", test_data[0].assign(variable="Foo", region="Moon")) c.add("test 2", store_ts, "target", "input 3") - # The computation fails - with pytest.raises(ComputationError, match="baz"): + # Succeeds with default strict=False + caplog.clear() + c.get("test 2") + + # A message is logged + r = caplog.record_tuples[-1] + assert ( + "ixmp.reporting.computations" == r[0] + and logging.ERROR == r[1] + and r[2].startswith("Failed with ValueError('region = Moon')") + ), caplog.record_tuples + + caplog.clear() + + # with strict=True, the computation fails + c.add("test 2", partial(store_ts, strict=True), "target", "input 3") + with pytest.raises( + ComputationError, + match=re.compile("computing 'test 2' using:.*region = Moon", flags=re.DOTALL), + ): c.get("test 2") diff --git a/ixmp/tests/reporting/test_reporter.py b/ixmp/tests/reporting/test_reporter.py index d37582ad3..4cb6ba235 100644 --- a/ixmp/tests/reporting/test_reporter.py +++ b/ixmp/tests/reporting/test_reporter.py @@ -166,7 +166,6 @@ def test_cli(ixmp_cli, test_mp, test_data_path): # TODO warning should be logged # Reporting produces the expected command-line output - assert re.match( "i j " # Trailing whitespace r""" @@ -176,8 +175,7 @@ def test_cli(ixmp_cli, test_mp, test_data_path): seattle chicago 1\.7 new-york 2\.5 topeka 1\.8 -(Name: value, )?dtype: float64 -""", +(Name: value, )?dtype: float64(, units: dimensionsless)?""", result.output, ) From d22f5ec15cfae10b695ace0e442f6f07a0cb4bd3 Mon Sep 17 00:00:00 2001 From: Paul Natsuo Kishimoto Date: Mon, 15 Aug 2022 17:41:48 +0200 Subject: [PATCH 8/8] Add #451 to release notes --- RELEASE_NOTES.rst | 4 ++++ doc/api-model.rst | 16 +++++++++++----- ixmp/model/base.py | 15 ++++++++++----- 3 files changed, 25 insertions(+), 10 deletions(-) diff --git a/RELEASE_NOTES.rst b/RELEASE_NOTES.rst index d6f44844a..658a943ce 100644 --- a/RELEASE_NOTES.rst +++ b/RELEASE_NOTES.rst @@ -4,6 +4,10 @@ Next release All changes ----------- +- Optionally tolerate failures to add individual items in :func:`.store_ts` reporting computation (:pull:`451`); use ``timeseries_only=True`` in check-out to function with :class:`.Scenario` with solution data stored. +- Bugfix: :class:`.Config` squashed configuration values read from :file:`config.json`, if the respective keys were registered in downstream packages, e.g. :mod:`message_ix`. + Allow the values loaded from file to persist (:pull:`451`). +- Adjust to genno 1.12 and set this as the minimum required version (:pull:`451`). - Add :meth:`.enforce` to the :class:`~.base.Model` API for enforcing structure/data consistency before :meth:`.Model.solve` (:pull:`450`). .. _v3.5.0: diff --git a/doc/api-model.rst b/doc/api-model.rst index 85f653175..94a2cfa26 100644 --- a/doc/api-model.rst +++ b/doc/api-model.rst @@ -32,16 +32,22 @@ Model API .. currentmodule:: ixmp.model.base .. autoclass:: ixmp.model.base.Model - :members: name, __init__, initialize, initialize_items, run + :members: name, __init__, run, initialize, initialize_items, enforce In the following, the words **required**, **optional**, etc. have specific meanings as described in `IETF RFC 2119 `_. - Model is an **abstract** class; this means it MUST be subclassed. - It has two REQURIED methods that MUST be overridden by subclasses: + Model is an *abstract* class; this means it **must** be subclassed. + It has two **required** methods that **must** be overridden by subclasses: .. autosummary:: - name __init__ + run + + The following attributes and methods are **optional** in subclasses. + The default implementations are either empty or implement reasonable default behaviour. + + .. autosummary:: + enforce initialize initialize_items - run + name diff --git a/ixmp/model/base.py b/ixmp/model/base.py index c5a07aad1..cce58b88b 100644 --- a/ixmp/model/base.py +++ b/ixmp/model/base.py @@ -14,12 +14,14 @@ class ModelError(Exception): class Model(ABC): #: Name of the model. - name = "base" + name: str = "base" @abstractmethod def __init__(self, name, **kwargs): """Constructor. + **Required.** + Parameters ---------- kwargs : @@ -38,12 +40,13 @@ def clean_path(cls, value: str) -> str: def enforce(scenario): """Enforce data consistency in `scenario`. - Optional. Implementations of :meth:`enforce`: + **Optional**; the default implementation does nothing. Subclass implementations + of :meth:`enforce`: - **should** modify the contents of sets and parameters so that `scenario` contains structure and data that is consistent with the underlying model. - **must not** add or remove sets or parameters; for that, use - :meth:`initiatize`. + :meth:`initialize`. :meth:`enforce` is always called by :meth:`run` before the model is run or solved; it **may** be called manually at other times. @@ -58,7 +61,8 @@ def enforce(scenario): def initialize(cls, scenario): """Set up *scenario* with required items. - Implementations of :meth:`initialize`: + **Optional**; the default implementation does nothing. Subclass implementations + of :meth:`initialize`: - **may** add sets, set elements, and/or parameter values. - **may** accept any number of keyword arguments to control behaviour. @@ -180,10 +184,11 @@ def initialize_items(cls, scenario, items): def run(self, scenario): """Execute the model. - Implementations of :meth:`run`: + **Required.** Implementations of :meth:`run`: - **must** call :meth:`enforce`. + Parameters ---------- scenario : .Scenario