diff --git a/CHANGELOG.md b/CHANGELOG.md index ce90c6d768..7533ecf0d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,17 @@ # [Unreleased](https://github.com/pybamm-team/PyBaMM/) +## Breaking changes + +- `Symbol.children`, `Symbol.domain` (and `secondary_domain`, `tertiary_domain`, `quaternary_domain`) and `FunctionParameter.input_names` return immutable tuples that compare equal to lists, and `Symbol.domains` returns a read-only copy, as do `to_json()`, `get_children_domains()`, `read_domain_or_domains()` and `pybamm.EMPTY_DOMAINS`. Editing them raises `TypeError` instead of emitting `SymbolMutationDeprecationWarning`, and not even `list` or `dict` methods can change a symbol through them. This ends the deprecation of these edits early; before 26.9 they left the symbol's identity stale. The sequences are no longer `list` instances and slicing them gives a plain tuple; tuples are accepted wherever domains are given. Build a new symbol with `with_domains`, `create_copy(new_children=...)` or `pybamm.replace`. The deprecated setters (`name`, `domains`, `scale`, ...) are unchanged. ([#5825](https://github.com/pybamm-team/PyBaMM/pull/5825)) + +## Deprecated + +- Setting `Array.entries_string` or `Interpolant.entries_string`, and calling `StateVector.set_evaluation_array`, are in-place symbol updates and now emit `SymbolMutationDeprecationWarning`. Construct a new symbol instead. ([#5825](https://github.com/pybamm-team/PyBaMM/pull/5825)) + +## Optimizations + +- Symbol mutation is no longer guarded by intercepting every attribute write, and `children` and `domain` return the stored immutable sequences instead of building a view on each access. Writes to a symbol's slots after construction are instead rejected by a static check of PyBaMM's code, tests, examples and documentation. Under the test suite's mutation guard, constructing an expression is twice as fast and parameterising it is 25% faster, which recovers most of the `test_parameterise` benchmark regression from [#5779](https://github.com/pybamm-team/PyBaMM/pull/5779). ([#5825](https://github.com/pybamm-team/PyBaMM/pull/5825)) + # [v26.9.0.0](https://github.com/pybamm-team/PyBaMM/tree/pybamm-v26.9.0.0) - 2026-09-28 ## Breaking changes diff --git a/docs/source/api/expression_tree/tree_util.rst b/docs/source/api/expression_tree/tree_util.rst index 4f4b144869..09abbf5ad5 100644 --- a/docs/source/api/expression_tree/tree_util.rst +++ b/docs/source/api/expression_tree/tree_util.rst @@ -30,8 +30,12 @@ Model parameter processing remains available through Migrating in-place updates -------------------------- -Public symbol setters and domain/list edits remain available but emit -``SymbolMutationDeprecationWarning``. Prefer constructing replacements:: +Public symbol setters remain available but emit +``SymbolMutationDeprecationWarning``. ``children``, ``domain`` and +``input_names`` return immutable tuples that compare equal to lists, and +``domains`` returns a read-only copy: editing them raises ``TypeError``, and not +even ``list`` or ``dict`` methods can change the symbol through them. Prefer +constructing replacements:: updated = variable.create_copy(scale=2, reference=0, bounds=(0, 1)) expression = pybamm.replace(expression, {variable: updated}) @@ -44,8 +48,7 @@ appropriate class. IDs update automatically; callers need not call ``set_id``. Deprecated updates invalidate cached IDs and derived values across live symbols. This deliberately slow compatibility path adds no per-symbol tracking to normal -replacement. Public domain and child-list views support deprecated edits while -internal storage remains shared. Mutating symbols already used as dictionary keys +replacement. Mutating symbols already used as dictionary keys cannot repair those dictionaries: use a fresh replacement memo and rebuild any parameter-processed or discretised model after legacy edits. Changes to a child's domains do not propagate into constructor-derived parent domains; reconstruct @@ -54,4 +57,9 @@ active because frozen ancestors cannot be discovered for cache invalidation; use out-of-place replacement or unfreeze the collector first. The test suite forbids deprecated mutation even with warnings ignored. Only isolated compatibility subprocesses exercise the legacy path; -notebook kernels inherit the same guard as other tests. +notebook kernels inherit the same guard as other tests. The guard +(``PYBAMM_TEST_FORBID_SYMBOL_MUTATION=1``) makes every deprecated public update +raise. A symbol's private slots, such as ``symbol._name``, are not part of the +API and writing them is unsupported: rather than intercept every attribute write +at runtime, the test suite checks PyBaMM's own code, tests, examples and +documentation for such writes statically. diff --git a/packages/pybamm/src/pybamm/discretisations/discretisation.py b/packages/pybamm/src/pybamm/discretisations/discretisation.py index 19621d37dc..aa8ebb502e 100644 --- a/packages/pybamm/src/pybamm/discretisations/discretisation.py +++ b/packages/pybamm/src/pybamm/discretisations/discretisation.py @@ -934,7 +934,7 @@ def _discretises_own_children(cls, symbol): def _discretise_node(self, symbol, new_leaves): """Discretise one node given its discretised children; the callback for :func:`pybamm.tree_map`.""" - if symbol._domains["primary"] != [] and self.bcs: + if symbol._domains["primary"] and self.bcs: # If boundary conditions are provided, need to check for BCs on tabs key_id = next(iter(self.bcs.keys())) if LEGACY_TAB_SIDES & set(self.bcs[key_id].keys()): @@ -951,7 +951,7 @@ def _discretise_node(self, symbol, new_leaves): def _spatial_method_of(self, symbol): """The spatial method of a symbol's primary domain (None if it has none).""" domain = symbol._domains["primary"] - if domain == []: + if not domain: return None return self.spatial_methods[domain[0]] diff --git a/packages/pybamm/src/pybamm/expression_tree/array.py b/packages/pybamm/src/pybamm/expression_tree/array.py index f88d5afd77..440dbb0ce9 100644 --- a/packages/pybamm/src/pybamm/expression_tree/array.py +++ b/packages/pybamm/src/pybamm/expression_tree/array.py @@ -64,7 +64,7 @@ def __init__( name = f"Array of shape {entries.shape!s}" self._entries = entries.astype(float) # Use known entries string to avoid re-hashing, where possible - self.entries_string = entries_string + self._entries_string = self._make_entries_string(entries_string) super().__init__( name, domain=domain, auxiliary_domains=auxiliary_domains, domains=domains ) @@ -109,22 +109,26 @@ def entries_string(self): @entries_string.setter def entries_string(self, value: tuple | None): + pybamm.expression_tree.symbol._warn_mutation( + "entries_string", "Construct a new Array with the desired entries." + ) + object.__setattr__(self, "_entries_string", self._make_entries_string(value)) + + def _make_entries_string(self, value: tuple | None) -> tuple: + """The hashable form of the entries: ``value`` if given, else computed.""" # We must include the entries in the hash, since different arrays can be # indistinguishable by class, name and domain alone - # Slightly different syntax for sparse and non-sparse matrices if value is not None: - self._entries_string = value - else: - entries = self._entries - if issparse(entries): - dct = entries.__dict__ - entries_string = ["shape", str(dct["_shape"])] - for key in ["data", "indices", "indptr"]: - entries_string += [key, dct[key].tobytes()] - self._entries_string = tuple(entries_string) - # self._entries_string = str(entries.__dict__) - else: - self._entries_string = (entries.tobytes(),) + return value + entries = self._entries + # Slightly different syntax for sparse and non-sparse matrices + if issparse(entries): + dct = entries.__dict__ + entries_string = ["shape", str(dct["_shape"])] + for key in ["data", "indices", "indptr"]: + entries_string += [key, dct[key].tobytes()] + return tuple(entries_string) + return (entries.tobytes(),) def _to_casadi(self, t, y, y_dot, inputs, casadi_symbols): """See :meth:`pybamm.Symbol._to_casadi()`.""" @@ -185,7 +189,7 @@ def to_json(self): json_dict = { "name": self.name, - "domains": self._domains, + "domains": self.domains, "entries": matrix, } diff --git a/packages/pybamm/src/pybamm/expression_tree/averages.py b/packages/pybamm/src/pybamm/expression_tree/averages.py index 1803b345fe..ed4d0c18b2 100644 --- a/packages/pybamm/src/pybamm/expression_tree/averages.py +++ b/packages/pybamm/src/pybamm/expression_tree/averages.py @@ -380,7 +380,7 @@ def __init__(self, child: pybamm.Symbol, f_a_dist) -> None: def to_json(self): return { "name": self.name, - "domains": self._domains, + "domains": self.domains, "children": [self._children[0], self.f_a_dist], } diff --git a/packages/pybamm/src/pybamm/expression_tree/binary_operators.py b/packages/pybamm/src/pybamm/expression_tree/binary_operators.py index ba276f7c0b..f6517ed67a 100644 --- a/packages/pybamm/src/pybamm/expression_tree/binary_operators.py +++ b/packages/pybamm/src/pybamm/expression_tree/binary_operators.py @@ -56,8 +56,8 @@ def _preprocess_binary( # Do some broadcasting in special cases, to avoid having to do this manually if ( - left._domains["primary"] != [] - and right._domains["primary"] != [] + left._domains["primary"] + and right._domains["primary"] and left._domains["primary"] != right._domains["primary"] ): if left._domains["primary"] == right._domains["secondary"]: @@ -92,7 +92,7 @@ def __init__( ) -> None: left, right = _preprocess_binary(left_child, right_child) - domains = self.get_children_domains([left, right]) + domains = self._combine_children_domains([left, right]) super().__init__(name, children=[left, right], domains=domains) @property @@ -106,7 +106,9 @@ def left(self, value: ChildSymbol) -> None: ) children = list(self._children) children[0] = pybamm.convert_to_symbol(value) - object.__setattr__(self, "_children", children) + object.__setattr__( + self, "_children", pybamm.expression_tree.symbol.SymbolChildren(children) + ) @property def right(self) -> pybamm.Symbol: @@ -119,7 +121,9 @@ def right(self, value: ChildSymbol) -> None: ) children = list(self._children) children[1] = pybamm.convert_to_symbol(value) - object.__setattr__(self, "_children", children) + object.__setattr__( + self, "_children", pybamm.expression_tree.symbol.SymbolChildren(children) + ) @classmethod def _from_json(cls, snippet: dict): @@ -264,7 +268,7 @@ def to_json(self): Method to serialise a BinaryOperator object into JSON. """ - json_dict = {"name": self.name, "domains": self._domains} + json_dict = {"name": self.name, "domains": self.domains} return json_dict @@ -1147,10 +1151,9 @@ def unpack_broadcast_recursive(symbol: pybamm.Symbol) -> pybamm.Symbol: return out return symbol - # No need to broadcast if the other symbol already has the shape that is being - # broadcasted to - # Do this recursively - if left._domains == right._domains: + # a broadcast to the other side's shape is redundant; interned domains are equal + # only if identical + if left._domains is right._domains: if isinstance(left, pybamm.Broadcast) and left.broadcasts_to_nodes: left = unpack_broadcast_recursive(left) elif isinstance(right, pybamm.Broadcast) and right.broadcasts_to_nodes: diff --git a/packages/pybamm/src/pybamm/expression_tree/broadcasts.py b/packages/pybamm/src/pybamm/expression_tree/broadcasts.py index 715afdcea1..cb548e8934 100644 --- a/packages/pybamm/src/pybamm/expression_tree/broadcasts.py +++ b/packages/pybamm/src/pybamm/expression_tree/broadcasts.py @@ -9,6 +9,7 @@ from scipy.sparse import csr_matrix import pybamm +from pybamm.expression_tree.symbol import _domain_list from pybamm.type_definitions import ( AuxiliaryDomainType, DomainsType, @@ -117,8 +118,7 @@ def __init__( child = cast(pybamm.Symbol, child) # Convert domain to list if it's a string - if isinstance(broadcast_domain, str): - broadcast_domain = [broadcast_domain] + broadcast_domain = _domain_list(broadcast_domain) # perform some basic checks and set attributes domains = self.check_and_set_domains(child, broadcast_domain) self.broadcast_domain = broadcast_domain @@ -192,7 +192,7 @@ def _evaluate_for_shape(self): def to_json(self): return { "name": self.name, - "domains": self._domains, + "domains": self.domains, "broadcast_domain": self.broadcast_domain, } @@ -260,8 +260,7 @@ def __init__( name: str | None = None, ): # Convert domain to list if it's a string - if isinstance(broadcast_domain, str): - broadcast_domain = [broadcast_domain] + broadcast_domain = _domain_list(broadcast_domain) # perform some basic checks and set attributes domains = self.check_and_set_domains(child, broadcast_domain) self.broadcast_domain = broadcast_domain @@ -342,7 +341,7 @@ def _evaluate_for_shape(self): def to_json(self): return { "name": self.name, - "domains": self._domains, + "domains": self.domains, "broadcast_domain": self.broadcast_domain, } @@ -410,8 +409,7 @@ def __init__( name: str | None = None, ): # Convert domain to list if it's a string - if isinstance(broadcast_domain, str): - broadcast_domain = [broadcast_domain] + broadcast_domain = _domain_list(broadcast_domain) # perform some basic checks and set attributes domains = self.check_and_set_domains(child, broadcast_domain) self.broadcast_domain = broadcast_domain @@ -479,7 +477,7 @@ def _evaluate_for_shape(self): def to_json(self): return { "name": self.name, - "domains": self._domains, + "domains": self.domains, "broadcast_domain": self.broadcast_domain, } @@ -541,9 +539,12 @@ def __init__( if isinstance(auxiliary_domains, str): auxiliary_domains = {"secondary": auxiliary_domains} - broadcast_domains = self.read_domain_or_domains( - broadcast_domain, auxiliary_domains, broadcast_domains - ) + broadcast_domains = { + level: _domain_list(names) + for level, names in self._read_domain_or_domains( + broadcast_domain, auxiliary_domains, broadcast_domains + ).items() + } # perform some basic checks and set attributes domains = self.check_and_set_domains(child, broadcast_domains) self.broadcast_domain = broadcast_domains["primary"] @@ -569,7 +570,7 @@ def _unary_new_copy(self, child, perform_simplifications=True): return self.__class__(child, broadcast_domains=self._domains) def to_json(self): - return {"name": self.name, "domains": self._domains} + return {"name": self.name, "domains": self.domains} @classmethod def _from_json(cls, snippet): diff --git a/packages/pybamm/src/pybamm/expression_tree/concatenations.py b/packages/pybamm/src/pybamm/expression_tree/concatenations.py index d7a1bcf904..23e162c81b 100644 --- a/packages/pybamm/src/pybamm/expression_tree/concatenations.py +++ b/packages/pybamm/src/pybamm/expression_tree/concatenations.py @@ -56,7 +56,7 @@ def __init__( if name is None: name = "concatenation" if check_domain: - domains = self.get_children_domains(children) + domains = self._combine_children_domains(children) else: domains = {"primary": []} self.concatenation_function = concat_fun @@ -96,7 +96,7 @@ def _diff(self, variable: pybamm.Symbol): return diff - def get_children_domains(self, children: Sequence[pybamm.Symbol]): + def _combine_children_domains(self, children: Sequence[pybamm.Symbol]): # combine domains from children domain: list = [] for child in children: @@ -495,7 +495,7 @@ def unpack_defaultDict(slices): json_dict = { "name": self.name, - "domains": self._domains, + "domains": self.domains, "slices": unpack_defaultDict(self._slices), "size": self._size, "children_slices": [ diff --git a/packages/pybamm/src/pybamm/expression_tree/conditional.py b/packages/pybamm/src/pybamm/expression_tree/conditional.py index 6d569e375b..5c15d731f0 100644 --- a/packages/pybamm/src/pybamm/expression_tree/conditional.py +++ b/packages/pybamm/src/pybamm/expression_tree/conditional.py @@ -30,7 +30,7 @@ def __init__(self, selector: pybamm.Symbol, *branches: pybamm.Symbol): if selector.shape_for_testing != (): raise ValueError("Conditional selector must evaluate to a scalar") - domains = self.get_children_domains(branches) + domains = self._combine_children_domains(branches) first_shape = branches[0].shape_for_testing first_size = branches[0].size_for_testing returns_scalar = first_size == 1 diff --git a/packages/pybamm/src/pybamm/expression_tree/discrete_time_sum.py b/packages/pybamm/src/pybamm/expression_tree/discrete_time_sum.py index 8b75f348c3..faeceed42d 100644 --- a/packages/pybamm/src/pybamm/expression_tree/discrete_time_sum.py +++ b/packages/pybamm/src/pybamm/expression_tree/discrete_time_sum.py @@ -33,7 +33,7 @@ def __init__( def to_json(self): return { "name": self.name, - "domains": self._domains, + "domains": self.domains, "time_points": self.x[0].tolist(), "data": self.y.tolist(), } diff --git a/packages/pybamm/src/pybamm/expression_tree/functions.py b/packages/pybamm/src/pybamm/expression_tree/functions.py index 574e3ace50..6cd8a6e7e7 100644 --- a/packages/pybamm/src/pybamm/expression_tree/functions.py +++ b/packages/pybamm/src/pybamm/expression_tree/functions.py @@ -70,7 +70,7 @@ def __init__( name = f"function ({function.__name__})" except AttributeError: name = f"function ({function.__class__})" - domains = self.get_children_domains(children) + domains = self._combine_children_domains(children) self.function = function self.differentiated_function = differentiated_function diff --git a/packages/pybamm/src/pybamm/expression_tree/independent_variable.py b/packages/pybamm/src/pybamm/expression_tree/independent_variable.py index 3cd1621ac9..38da4d914c 100644 --- a/packages/pybamm/src/pybamm/expression_tree/independent_variable.py +++ b/packages/pybamm/src/pybamm/expression_tree/independent_variable.py @@ -198,7 +198,7 @@ def create_copy( def to_json(self): return { "name": self.name, - "domains": self._domains, + "domains": self.domains, "coord_sys": self.coord_sys, "direction": getattr(self, "direction", None), } diff --git a/packages/pybamm/src/pybamm/expression_tree/interpolant.py b/packages/pybamm/src/pybamm/expression_tree/interpolant.py index 5870602728..b77f0a5c71 100644 --- a/packages/pybamm/src/pybamm/expression_tree/interpolant.py +++ b/packages/pybamm/src/pybamm/expression_tree/interpolant.py @@ -160,7 +160,7 @@ def __init__( name = "interpolating_function" self.x = x self.y = y - self.entries_string = entries_string + self._entries_string = self._make_entries_string(entries_string) # Differentiate the interpolating function if necessary self._num_derivatives = _num_derivatives @@ -199,16 +199,21 @@ def entries_string(self): @entries_string.setter def entries_string(self, value): - # We must include the entries in the hash, since different arrays can be + pybamm.expression_tree.symbol._warn_mutation( + "entries_string", "Construct a new Interpolant with the desired data." + ) + object.__setattr__(self, "_entries_string", self._make_entries_string(value)) + + def _make_entries_string(self, value: str | None) -> str: + """The hashable form of the data: ``value`` if given, else computed.""" + # We must include the data in the hash, since different interpolants can be # indistinguishable by class, name and domain alone - # Slightly different syntax for sparse and non-sparse matrices if value is not None: - self._entries_string = value - else: - self._entries_string = "" - for i, x in enumerate(self.x): - self._entries_string += "x" + str(i) + "_" + str(x.tobytes()) - self._entries_string += "y_" + str(self.y.tobytes()) + return value + entries_string = "" + for i, x in enumerate(self.x): + entries_string += "x" + str(i) + "_" + str(x.tobytes()) + return entries_string + "y_" + str(self.y.tobytes()) def create_copy(self, new_children=None, perform_simplifications=True): """See :meth:`pybamm.Symbol.new_copy()`.""" diff --git a/packages/pybamm/src/pybamm/expression_tree/legacy_mutation.py b/packages/pybamm/src/pybamm/expression_tree/legacy_mutation.py index b604f45fd4..348331e3d9 100644 --- a/packages/pybamm/src/pybamm/expression_tree/legacy_mutation.py +++ b/packages/pybamm/src/pybamm/expression_tree/legacy_mutation.py @@ -5,9 +5,7 @@ import gc import operator import os -import sys import warnings -from typing import Any from pybamm.expression_tree.tree_util import _TRANSIENT_SLOTS @@ -20,11 +18,6 @@ class SymbolMutationDeprecationWarning(DeprecationWarning): MUTATION_FORBIDDEN = os.environ.get("PYBAMM_TEST_FORBID_SYMBOL_MUTATION") == "1" _initial_frozen_count = gc.get_freeze_count() -# Display-only metadata, not part of identity or evaluation -_DISPLAY_ATTRIBUTES = frozenset( - {"_print_name", "_raw_print_name", "domain", "print_name"} -) - def _warn_mutation(operation: str, replacement: str) -> None: message = ( @@ -98,228 +91,3 @@ def upgrade_pickled_state(cls: type, state: dict) -> dict: if upgraded.get("_raw_print_name") is None: upgraded["_raw_print_name"] = state.get("_print_name") return upgraded - - -def _intern_domains(domains): - from pybamm.expression_tree.symbol import _intern_domains - - return _intern_domains(domains) - - -class _DomainList(list): - """A list view supporting deprecated edits to its owning symbol.""" - - __slots__ = ("_level", "_owner", "_source") - - def __init__(self, owner: Any, level: str): - self._owner, self._level = owner, level - self._source = self._read_values() - super().__init__(self._source) - - def _refresh(self) -> None: - values = self._read_values() - if values is not self._source: - list.__setitem__(self, slice(None), values) - self._source = values - - def _read_values(self): - if self._owner is not None and self._level not in self._owner._domains: - self._owner = None - return ( - self._owner._domains[self._level] - if self._owner is not None - else list.copy(self) - ) - - def _write_values(self, values) -> None: - if self._owner is None: - return - domains = dict(self._owner._domains) - domains[self._level] = values - object.__setattr__(self._owner, "_domains", _intern_domains(domains)) - - def __reduce__(self): - return (list, (list(self),)) - - -def _list_mutator(name): - def mutate(self, *args, **kwargs): - replacement = ( - "Build a new symbol with the desired children or inputs." - if isinstance(self, _SymbolList) - else "Build a new symbol with with_domains()." - ) - _warn_mutation(f"{self._level}.{name}", replacement) - values = list(self) - result = getattr(values, name)(*args, **kwargs) - self._write_values(values) - list.__setitem__(self, slice(None), values) - return self if name in ("__iadd__", "__imul__") else result - - return mutate - - -for _method in ( - "__setitem__", - "__delitem__", - "__iadd__", - "__imul__", - "append", - "extend", - "insert", - "pop", - "remove", - "clear", - "sort", - "reverse", -): - setattr(_DomainList, _method, _list_mutator(_method)) - - -class _SymbolList(_DomainList): - """A list view for legacy edits of symbol children and input names.""" - - __slots__ = () - - def _read_values(self): - return getattr(self._owner, self._level) - - def _write_values(self, values) -> None: - object.__setattr__(self._owner, self._level, values) - - -class _DomainsDict(dict): - """A dictionary view supporting deprecated edits to its owning symbol.""" - - __slots__ = ("_owner", "_source") - - def __init__(self, owner: Any): - super().__init__((level, _DomainList(owner, level)) for level in owner._domains) - self._owner = owner - self._source = owner._domains - - def _refresh(self) -> None: - if self._owner._domains is not self._source: - dict.clear(self) - dict.update( - self, - ( - (level, _DomainList(self._owner, level)) - for level in self._owner._domains - ), - ) - self._source = self._owner._domains - - def __reduce__(self): - return (dict, ({level: list(names) for level, names in self.items()},)) - - -def _dict_mutator(name): - def mutate(self, *args, **kwargs): - self._refresh() - domains = {level: list(names) for level, names in dict.items(self)} - _warn_mutation(f"domains.{name}", "Build a new symbol with with_domains().") - result = getattr(domains, name)(*args, **kwargs) - object.__setattr__(self._owner, "_domains", _intern_domains(domains)) - dict.clear(self) - dict.update( - self, ((level, _DomainList(self._owner, level)) for level in domains) - ) - self._source = self._owner._domains - if name == "setdefault": - return self[args[0]] - return self if name == "__ior__" else result - - return mutate - - -for _method in ( - "__setitem__", - "__delitem__", - "clear", - "pop", - "popitem", - "setdefault", - "update", - "__ior__", -): - setattr(_DomainsDict, _method, _dict_mutator(_method)) - - -def _domain_reader(method): - def read(self, *args, **kwargs): - self._refresh() - for arg in args: - if isinstance(arg, _DomainList | _DomainsDict): - arg._refresh() - return method(self, *args, **kwargs) - - return read - - -for _method in ( - "__getitem__", - "__iter__", - "__len__", - "__contains__", - "__repr__", - "__eq__", - "__ne__", - "__lt__", - "__le__", - "__gt__", - "__ge__", - "__add__", - "__mul__", - "__rmul__", - "__reversed__", - "count", - "index", - "copy", -): - setattr(_DomainList, _method, _domain_reader(getattr(list, _method))) - -for _method in ( - "__getitem__", - "__iter__", - "__len__", - "__contains__", - "__repr__", - "__eq__", - "__ne__", - "__or__", - "__ror__", - "__reversed__", - "get", - "items", - "keys", - "values", - "copy", -): - setattr(_DomainsDict, _method, _domain_reader(getattr(dict, _method))) - - -def _in_own_constructor(symbol) -> bool: - """Whether an ``__init__`` of ``symbol`` is on the calling thread's stack.""" - frame = sys._getframe(2) - while frame is not None: - if frame.f_code.co_name == "__init__" and frame.f_locals.get("self") is symbol: - return True - frame = frame.f_back - return False - - -def _frozen_setattr(self, name, value): - """``Symbol.__setattr__`` while mutation is forbidden.""" - if ( - name not in _TRANSIENT_SLOTS - and name not in _DISPLAY_ATTRIBUTES - and hasattr(self, "_id") # set once Symbol.__init__ has run - and not _in_own_constructor(self) - ): - raise AttributeError( - f"Cannot set '{name}' on {type(self).__name__} '{self.name}': symbols " - "are immutable once constructed. Build a new symbol instead (e.g. " - "`create_copy`, `with_domains`)." - ) - object.__setattr__(self, name, value) diff --git a/packages/pybamm/src/pybamm/expression_tree/parameter.py b/packages/pybamm/src/pybamm/expression_tree/parameter.py index ebc48b1bc3..6873315168 100644 --- a/packages/pybamm/src/pybamm/expression_tree/parameter.py +++ b/packages/pybamm/src/pybamm/expression_tree/parameter.py @@ -10,6 +10,17 @@ import sympy import pybamm +from pybamm.expression_tree.symbol import _ImmutableSequence + + +class InputNames(_ImmutableSequence): + """An immutable list of a function parameter's input names.""" + + __slots__ = () + _immutable_message = ( + "Function parameter input names are immutable; construct a new " + "FunctionParameter with the desired inputs" + ) class Parameter(pybamm.Symbol): @@ -58,7 +69,7 @@ def to_equation(self) -> sympy.Symbol: return sympy.Symbol(self.name) def to_json(self): - return {"name": self.name, "domains": self._domains} + return {"name": self.name, "domains": self.domains} @classmethod def _from_json(cls, snippet): @@ -119,10 +130,10 @@ def __init__( if isinstance(child, float | int | np.number): children_list[idx] = pybamm.Scalar(child) - domains = self.get_children_domains(children_list) + domains = self._combine_children_domains(children_list) super().__init__(name, children=children_list, domains=domains) - self._input_names = self._check_input_names(list(inputs.keys())) + self._input_names = InputNames(self._check_input_names(list(inputs.keys()))) # Use the inspect module to find the function's "short name" from the # Parameters module that called it @@ -155,21 +166,27 @@ def print_input_names(self): print(inp) @property - def input_names(self) -> list[str]: - return ( - pybamm.expression_tree.symbol._SymbolList(self, "_input_names") - if isinstance(self._input_names, list) - else self._input_names - ) + def input_names(self) -> InputNames: + """The names of the function's inputs, as an immutable list.""" + return self._input_names @input_names.setter def input_names(self, value: list[str]) -> None: pybamm.expression_tree.symbol._warn_mutation( "input_names", "Construct a new FunctionParameter with the desired inputs." ) - if isinstance(value, pybamm.expression_tree.symbol._SymbolList): + if isinstance(value, InputNames): value = list(value) - object.__setattr__(self, "_input_names", self._check_input_names(value)) + object.__setattr__( + self, "_input_names", InputNames(self._check_input_names(value) or []) + ) + + def __setstate__(self, state): + super().__setstate__(state) + names = state.get("_input_names") + if names is not None and type(names) is not InputNames: + # pickled when input names were stored as plain lists + object.__setattr__(self, "_input_names", InputNames(names)) @staticmethod def _check_input_names(inp): @@ -247,7 +264,7 @@ def to_json(self): children = [*children, self.diff_variable] return { "name": self.name, - "domains": self._domains, + "domains": self.domains, "input_names": list(self.input_names), "print_name": self.print_name, "has_diff_variable": self.diff_variable is not None, diff --git a/packages/pybamm/src/pybamm/expression_tree/state_vector.py b/packages/pybamm/src/pybamm/expression_tree/state_vector.py index 20d4c9681a..3c0603a688 100644 --- a/packages/pybamm/src/pybamm/expression_tree/state_vector.py +++ b/packages/pybamm/src/pybamm/expression_tree/state_vector.py @@ -70,7 +70,7 @@ def __init__( self._y_slices = y_slices self._first_point = y_slices[0].start self._last_point = y_slices[-1].stop - self.set_evaluation_array(y_slices, evaluation_array) + self._evaluation_array = self._make_evaluation_array(y_slices, evaluation_array) super().__init__( name=name, domain=domain, @@ -111,14 +111,27 @@ def size(self): return self.evaluation_array.count(True) def set_evaluation_array(self, y_slices, evaluation_array): - """Set evaluation array using slices.""" + """Set evaluation array using slices (deprecated).""" + pybamm.expression_tree.symbol._warn_mutation( + "set_evaluation_array", + "Construct a new StateVector with the desired slices.", + ) + object.__setattr__( + self, + "_evaluation_array", + self._make_evaluation_array(y_slices, evaluation_array), + ) + + @staticmethod + def _make_evaluation_array(y_slices, evaluation_array) -> list[bool]: + """The evaluation array: ``evaluation_array`` if given, else computed from + the slices.""" if evaluation_array is not None and pybamm.settings.debug_mode is False: - self._evaluation_array = evaluation_array - else: - array = np.zeros(y_slices[-1].stop) - for y_slice in y_slices: - array[y_slice] = True - self._evaluation_array = [bool(x) for x in array] + return evaluation_array + array = np.zeros(y_slices[-1].stop) + for y_slice in y_slices: + array[y_slice] = True + return [bool(x) for x in array] def _jac_diff_vector(self, variable: pybamm.StateVectorBase): """ @@ -214,7 +227,7 @@ def to_json(self): json_dict = { "name": self.name, - "domains": self._domains, + "domains": self.domains, "y_slice": [ { "start": y.start, diff --git a/packages/pybamm/src/pybamm/expression_tree/symbol.py b/packages/pybamm/src/pybamm/expression_tree/symbol.py index b3aed50b6f..2adf8975af 100644 --- a/packages/pybamm/src/pybamm/expression_tree/symbol.py +++ b/packages/pybamm/src/pybamm/expression_tree/symbol.py @@ -15,11 +15,6 @@ import pybamm from pybamm.expression_tree.legacy_mutation import ( - MUTATION_FORBIDDEN, - _DomainList, - _DomainsDict, - _frozen_setattr, - _SymbolList, _warn_mutation, legacy_property, upgrade_pickled_state, @@ -47,19 +42,78 @@ def _immutable(self, *args, **kwargs): ) -class DomainNames(list): - """An immutable list of domain names (compares equal to plain lists).""" +class _ImmutableSequence(tuple): + """ + A tuple that compares equal to, concatenates with and prints like a list. + Being a tuple, no method (including those of ``list``) can edit it in place; + the list mutators raise ``TypeError``. Subclasses set ``_immutable_message`` + to say how to build a new value. + """ __slots__ = () + _immutable_message = "This sequence is immutable" + + def _immutable(self, *args, **kwargs): + raise TypeError(self._immutable_message) + __setitem__ = __delitem__ = __iadd__ = __imul__ = _immutable append = extend = insert = pop = remove = clear = sort = reverse = _immutable - def copy(self) -> list[str]: + def __eq__(self, other): + if isinstance(other, list): + other = tuple(other) + return tuple.__eq__(self, other) + + def __ne__(self, other): + if isinstance(other, list): + other = tuple(other) + return tuple.__ne__(self, other) + + __hash__ = tuple.__hash__ + + def __add__(self, other): + if isinstance(other, list | tuple): + return [*self, *other] + return NotImplemented + + def __radd__(self, other): + if isinstance(other, list): + return [*other, *self] + return NotImplemented + + def __mul__(self, count): + return list(self) * count + + __rmul__ = __mul__ + + def __repr__(self): + return repr(list(self)) + + def copy(self) -> list: """A mutable plain-list copy.""" return list(self) def __reduce__(self): - return (DomainNames, (list(self),)) + return (type(self), (tuple(self),)) + + +class DomainNames(_ImmutableSequence): + """An immutable list of domain names (compares equal to plain lists).""" + + __slots__ = () + _immutable_message = ( + "Symbol domains are immutable; build a new symbol with `with_domains`" + ) + + +class SymbolChildren(_ImmutableSequence): + """An immutable list of a symbol's children (compares equal to plain lists).""" + + __slots__ = () + _immutable_message = ( + "Symbol children are immutable; build a new symbol with " + "`create_copy(new_children=...)` or `pybamm.replace`" + ) class Domains(dict): @@ -93,6 +147,25 @@ def __reduce__(self): return (_intern_domains, (dict(self),)) +class ReadOnlyDomains(dict): + """ + A read-only copy of a symbol's domains, as returned by + :attr:`Symbol.domains`. Editing it raises ``TypeError``; it never shares + storage with the symbol, so not even ``dict`` methods can change the symbol. + """ + + __slots__ = () + __setitem__ = __delitem__ = clear = pop = popitem = setdefault = _immutable + update = __ior__ = _immutable + + def copy(self) -> dict[str, DomainNames]: + """A mutable plain-dict copy.""" + return dict(self) + + def __reduce__(self): + return (ReadOnlyDomains, (dict(self),)) + + _INTERNED_DOMAINS: dict[tuple, Domains] = {} @@ -123,12 +196,18 @@ def _intern_domains(domains: dict[str, Sequence[str]]) -> Domains: return interned -EMPTY_DOMAINS: Domains = _intern_domains({k: [] for k in DOMAIN_LEVELS}) +_EMPTY_DOMAINS: Domains = _intern_domains({k: [] for k in DOMAIN_LEVELS}) +EMPTY_DOMAINS: ReadOnlyDomains = ReadOnlyDomains(_EMPTY_DOMAINS) # raw domains input (as given to a constructor) -> validated, interned domains _VALIDATED_DOMAINS: dict[tuple, Domains] = {} +def _domain_list(domain: str | Sequence[str]) -> list[str]: + """A domain as a plain list of names, whichever sequence it was given as.""" + return [domain] if isinstance(domain, str) else list(domain) + + def domain_size(domain: list[str] | str): """ Get the domain size. @@ -148,7 +227,7 @@ def domain_size(domain: list[str] | str): "negative particle size": 19, "positive particle size": 23, } - if domain in [[], None]: + if not domain: size = 1 elif all(dom in fixed_domain_sizes for dom in domain): size = sum(fixed_domain_sizes[dom] for dom in domain) @@ -380,10 +459,12 @@ def __init__( if children is None: children = [] - self._children = children + self._children = ( + children if type(children) is SymbolChildren else SymbolChildren(children) + ) self._domains = self._normalise_domains( - self.read_domain_or_domains(domain, auxiliary_domains, domains) + self._read_domain_or_domains(domain, auxiliary_domains, domains) ) # Test shape on everything but nodes that contain the base Symbol class or @@ -410,18 +491,9 @@ def _from_json(cls, snippet: dict): ) @property - def children(self) -> Sequence[Symbol]: - """ - returns the cached children of this node. - - Note: it is assumed that children of a node are not modified after initial - creation - """ - return ( - _SymbolList(self, "_children") - if isinstance(self._children, list) - else self._children - ) + def children(self) -> SymbolChildren: + """The children of this node, as an immutable list.""" + return self._children # Slots (besides ``_children``) holding symbols this node depends on: a symbol, # ``None``, or a list/tuple of symbols. Subclasses extend this. @@ -449,8 +521,9 @@ def name(self, value: str): object.__setattr__(self, "_name", value) @property - def domains(self): - return _DomainsDict(self) + def domains(self) -> ReadOnlyDomains: + """A read-only copy of the domains of this node.""" + return ReadOnlyDomains(self._domains) @domains.setter def domains(self, domains): @@ -465,7 +538,7 @@ def copy_domains(self, symbol: Symbol) -> None: def clear_domains(self) -> None: """Clear domains in place (deprecated).""" _warn_mutation("clear_domains", "Use symbol = symbol.without_domains().") - object.__setattr__(self, "_domains", EMPTY_DOMAINS) + object.__setattr__(self, "_domains", _EMPTY_DOMAINS) def set_id(self) -> None: """Recompute identity after a legacy update (deprecated).""" @@ -502,10 +575,10 @@ def _normalise_domains(domains: dict) -> Domains: def _validate_domains(domains: dict) -> Domains: # Turn dictionary into appropriate form if domains == {"primary": []}: - return EMPTY_DOMAINS + return _EMPTY_DOMAINS # Set default domains - domains = {**EMPTY_DOMAINS, **domains} + domains = {**_EMPTY_DOMAINS, **domains} # Check domains don't clash for level, dom in domains.items(): @@ -513,8 +586,8 @@ def _validate_domains(domains: dict) -> Domains: raise pybamm.DomainError( f"Domain keys must be one of '{DOMAIN_LEVELS}'" ) - if isinstance(dom, str): - domains[level] = [dom] + # tuples (e.g. slices of a symbol's domain) compare unequal to lists + domains[level] = _domain_list(dom) values = [tuple(val) for val in domains.values() if val != []] if len(set(values)) != len(values): @@ -538,7 +611,7 @@ def domain(self): ------- iterable of str """ - return _DomainList(self, "primary") + return self._domains["primary"] @domain.setter def domain(self, domain): @@ -556,17 +629,17 @@ def auxiliary_domains(self): @property def secondary_domain(self): """Helper function to get the secondary domain of a symbol.""" - return _DomainList(self, "secondary") + return self._domains["secondary"] @property def tertiary_domain(self): """Helper function to get the tertiary domain of a symbol.""" - return _DomainList(self, "tertiary") + return self._domains["tertiary"] @property def quaternary_domain(self): """Helper function to get the quaternary domain of a symbol.""" - return _DomainList(self, "quaternary") + return self._domains["quaternary"] mesh = legacy_property( "_mesh", @@ -616,7 +689,7 @@ def with_domains(self, domains: Symbol | dict[str, list[str] | str]) -> Symbol: def without_domains(self) -> Symbol: """Return a copy of this symbol with all domains cleared.""" - return self.with_domains(EMPTY_DOMAINS) + return self.with_domains(_EMPTY_DOMAINS) def with_mesh( self, @@ -650,19 +723,24 @@ def with_meshes( ("_secondary_mesh", domains["secondary"]), ("_tertiary_mesh", domains["tertiary"]), ) - if domain != [] + if domain } if not meshes: return self return self._replace(**meshes) - def get_children_domains(self, children: Sequence[Symbol]): + def get_children_domains(self, children: Sequence[Symbol]) -> dict: """Combine domains from children, at all levels.""" + domains = self._combine_children_domains(children) + return ReadOnlyDomains(domains) if type(domains) is Domains else domains + + def _combine_children_domains(self, children: Sequence[Symbol]): + """:meth:`get_children_domains`, returning the interned domains as-is.""" # fast path: every child with a domain shares the same (interned) domains shared = None for child in children: child_domains = child._domains - if child_domains is EMPTY_DOMAINS: + if child_domains is _EMPTY_DOMAINS: continue if shared is None: shared = child_domains @@ -670,7 +748,7 @@ def get_children_domains(self, children: Sequence[Symbol]): shared = False break if shared is None: - return EMPTY_DOMAINS + return _EMPTY_DOMAINS if shared is not False: return shared domains: dict = {} @@ -697,10 +775,21 @@ def read_domain_or_domains( domain: DomainType, auxiliary_domains: AuxiliaryDomainType, domains: DomainsType, + ) -> dict: + """Combine ``domain`` and ``auxiliary_domains``, or check ``domains``.""" + result = self._read_domain_or_domains(domain, auxiliary_domains, domains) + return ReadOnlyDomains(result) if type(result) is Domains else result + + def _read_domain_or_domains( + self, + domain: DomainType, + auxiliary_domains: AuxiliaryDomainType, + domains: DomainsType, ): + """:meth:`read_domain_or_domains`, returning the interned domains as-is.""" if domains is None: if domain is None and not auxiliary_domains: - return EMPTY_DOMAINS + return _EMPTY_DOMAINS if isinstance(domain, str): domain = [domain] elif domain is None: @@ -794,6 +883,10 @@ def __setstate__(self, state): object.__setattr__(self, key, value) for slot in pybamm.expression_tree.tree_util._TRANSIENT_SLOTS: object.__setattr__(self, slot, None) + children = state.get("_children") + if children is not None and type(children) is not SymbolChildren: + # pickled when children were stored as plain lists + object.__setattr__(self, "_children", SymbolChildren(children)) orphans = children @@ -902,7 +995,7 @@ def __str__(self): def __repr__(self): """returns the string `__class__(id, name, children, domain)`""" - return f"{self.__class__.__name__!s}({hex(self.id)}, {self._name!s}, children={[str(child) for child in self._children]!s}, domains={ ({k: v for k, v in self._domains.items() if v != []})!s})" + return f"{self.__class__.__name__!s}({hex(self.id)}, {self._name!s}, children={[str(child) for child in self._children]!s}, domains={ ({k: v for k, v in self._domains.items() if v})!s})" def __add__(self, other: ChildSymbol) -> pybamm.Addition: """return an :class:`Addition` object.""" @@ -1507,7 +1600,7 @@ def to_json(self): json_dict = { "name": self.name, - "domains": self._domains, + "domains": self.domains, } return json_dict @@ -1535,7 +1628,3 @@ def convert_to_symbol(value) -> Symbol: return value * pybamm.Scalar(1) except (NotImplementedError, TypeError, ValueError): raise ValueError("Input cannot be converted to a `pybamm.Symbol`") from None - - -if MUTATION_FORBIDDEN: - Symbol.__setattr__ = _frozen_setattr diff --git a/packages/pybamm/src/pybamm/expression_tree/tensor_field.py b/packages/pybamm/src/pybamm/expression_tree/tensor_field.py index a4f048ddbd..08421dd2cc 100644 --- a/packages/pybamm/src/pybamm/expression_tree/tensor_field.py +++ b/packages/pybamm/src/pybamm/expression_tree/tensor_field.py @@ -146,7 +146,7 @@ def create_copy( def to_json(self): return { "name": self.name, - "domains": self._domains, + "domains": self.domains, "children": list(self.components), } diff --git a/packages/pybamm/src/pybamm/expression_tree/tree_util.py b/packages/pybamm/src/pybamm/expression_tree/tree_util.py index d180d1d49e..af7e6f1d7a 100644 --- a/packages/pybamm/src/pybamm/expression_tree/tree_util.py +++ b/packages/pybamm/src/pybamm/expression_tree/tree_util.py @@ -104,7 +104,7 @@ def _hashable(value: Any): return value -def leaves_of(symbol: pybamm.Symbol) -> list[pybamm.Symbol]: +def leaves_of(symbol: pybamm.Symbol) -> Sequence[pybamm.Symbol]: """The symbols ``symbol`` directly depends on, children first.""" leaf_fields = layout(type(symbol))[0] if len(leaf_fields) == 1: diff --git a/packages/pybamm/src/pybamm/expression_tree/unary_operators.py b/packages/pybamm/src/pybamm/expression_tree/unary_operators.py index 57b847270c..fc376d7b0c 100644 --- a/packages/pybamm/src/pybamm/expression_tree/unary_operators.py +++ b/packages/pybamm/src/pybamm/expression_tree/unary_operators.py @@ -10,7 +10,7 @@ from scipy.sparse import csr_matrix, issparse import pybamm -from pybamm.expression_tree.symbol import EMPTY_DOMAINS +from pybamm.expression_tree.symbol import _EMPTY_DOMAINS from pybamm.type_definitions import ChildSymbol, DomainsType from pybamm.util import import_optional_dependency @@ -57,7 +57,9 @@ def child(self, value: ChildSymbol) -> None: ) children = list(self._children) children[0] = pybamm.convert_to_symbol(value) - object.__setattr__(self, "_children", children) + object.__setattr__( + self, "_children", pybamm.expression_tree.symbol.SymbolChildren(children) + ) def to_json(self): """See :meth:`pybamm.Symbol.to_json()`; extra symbol fields go in children.""" @@ -424,7 +426,7 @@ def __init__(self, child, index, name=None, check_size=True): # no domain for integer value key super().__init__( - name, child, domains=EMPTY_DOMAINS if isinstance(index, int) else None + name, child, domains=_EMPTY_DOMAINS if isinstance(index, int) else None ) @classmethod @@ -936,7 +938,7 @@ def __init__(self, child, vector_type="row"): name = "basis integral" self.vector_type = vector_type # integrating removes the domain - super().__init__(name, child, domains=EMPTY_DOMAINS) + super().__init__(name, child, domains=_EMPTY_DOMAINS) def _unary_new_copy(self, child, perform_simplifications=True): """See :meth:`UnaryOperator._unary_new_copy()`.""" @@ -1010,7 +1012,7 @@ def __init__(self, child, region="entire", name=None): self.region = region # boundary integral removes domains super().__init__( - name if given_name is None else given_name, child, EMPTY_DOMAINS + name if given_name is None else given_name, child, _EMPTY_DOMAINS ) def _unary_new_copy(self, child, perform_simplifications=True): @@ -1046,7 +1048,7 @@ def __init__(self, child, integration_domain, direction, region=None): def to_json(self): return { "name": self.name, - "domains": self._domains, + "domains": self.domains, "integration_domain": self.integration_domain, "direction": self.direction, "region": self.region, @@ -1097,7 +1099,7 @@ def __init__(self, child, side, domain): def to_json(self): return { "name": self.name, - "domains": self._domains, + "domains": self.domains, "side": self.side, "domain": self._domains["primary"], } @@ -1460,7 +1462,7 @@ def __init__(self, child, direction): def to_json(self): return { "name": self.name, - "domains": self._domains, + "domains": self.domains, "direction": self.direction, } @@ -1494,7 +1496,7 @@ def __init__(self, child, index): def to_json(self): return { "name": self.name, - "domains": self._domains, + "domains": self.domains, "index": self.index, } diff --git a/packages/pybamm/src/pybamm/expression_tree/variable.py b/packages/pybamm/src/pybamm/expression_tree/variable.py index 13d117e4fe..8bce4ef859 100644 --- a/packages/pybamm/src/pybamm/expression_tree/variable.py +++ b/packages/pybamm/src/pybamm/expression_tree/variable.py @@ -150,7 +150,7 @@ def to_equation(self): def to_json(self): return { "name": self.name, - "domains": self._domains, + "domains": self.domains, "children": [self._scale, self._reference, self.bounds[0], self.bounds[1]], "print_name": self._raw_print_name, } diff --git a/packages/pybamm/src/pybamm/parameters/parameter_substitutor.py b/packages/pybamm/src/pybamm/parameters/parameter_substitutor.py index bb224bb75a..a4968167fb 100644 --- a/packages/pybamm/src/pybamm/parameters/parameter_substitutor.py +++ b/packages/pybamm/src/pybamm/parameters/parameter_substitutor.py @@ -341,7 +341,7 @@ def rebind_domains(node, new_leaves): if node in placeholders: return new_node.with_domains(placeholders[node]) return new_node.with_domains( - new_node.get_children_domains(new_node.children) + new_node._combine_children_domains(new_node.children) ) expression = pybamm.tree_map(rebind_domains, expression) diff --git a/packages/pybamm/src/pybamm/spatial_methods/spatial_method.py b/packages/pybamm/src/pybamm/spatial_methods/spatial_method.py index 7a0c69b7b4..cf2f1a795d 100644 --- a/packages/pybamm/src/pybamm/spatial_methods/spatial_method.py +++ b/packages/pybamm/src/pybamm/spatial_methods/spatial_method.py @@ -46,7 +46,7 @@ def _get_auxiliary_domain_repeats(self, domains): """ mesh_pts = 1 for level, dom in domains.items(): - if level != "primary" and dom != []: + if level != "primary" and dom: mesh_pts *= self.mesh[dom].npts return mesh_pts diff --git a/packages/pybamm/tests/unit/test_expression_tree/test_broadcasts.py b/packages/pybamm/tests/unit/test_expression_tree/test_broadcasts.py index 1408fbfad4..837e420ee8 100644 --- a/packages/pybamm/tests/unit/test_expression_tree/test_broadcasts.py +++ b/packages/pybamm/tests/unit/test_expression_tree/test_broadcasts.py @@ -178,6 +178,27 @@ def test_tertiary_broadcast(self): with pytest.raises(pybamm.DomainError, match=r"Cannot do tertiary broadcast"): pybamm.TertiaryBroadcast(a, "negative electrode") + def test_broadcast_domains_accept_any_sequence_of_names(self): + particle = pybamm.Symbol("c", domain="negative particle") + in_electrode = pybamm.Symbol( + "c", + domain="negative particle", + auxiliary_domains={"secondary": "negative electrode"}, + ) + broadcasts = [ + (pybamm.PrimaryBroadcast, pybamm.Scalar(1), ["negative electrode"]), + (pybamm.SecondaryBroadcast, particle, ["negative electrode"]), + (pybamm.TertiaryBroadcast, in_electrode, ["current collector"]), + ] + for broadcast, child, domain in broadcasts: + assert broadcast(child, tuple(domain)) == broadcast(child, domain) + full = pybamm.FullBroadcast( + 1, broadcast_domains={"primary": ["negative electrode"], "secondary": []} + ) + assert full == pybamm.FullBroadcast( + 1, broadcast_domains={"primary": ("negative electrode",), "secondary": ()} + ) + def test_full_broadcast(self): a = pybamm.Symbol("a") broad_a = pybamm.FullBroadcast(a, ["negative electrode"], "current collector") diff --git a/packages/pybamm/tests/unit/test_expression_tree/test_legacy_mutation.py b/packages/pybamm/tests/unit/test_expression_tree/test_legacy_mutation.py index d04a0dd302..665f4e852f 100644 --- a/packages/pybamm/tests/unit/test_expression_tree/test_legacy_mutation.py +++ b/packages/pybamm/tests/unit/test_expression_tree/test_legacy_mutation.py @@ -1,14 +1,30 @@ from __future__ import annotations +import ast +import copy +import functools +import heapq +import json +import operator import os +import pickle import subprocess import sys import textwrap import warnings +from pathlib import Path +import numpy as np import pytest import pybamm +from pybamm.expression_tree.parameter import InputNames +from pybamm.expression_tree.symbol import ( + DomainNames, + ReadOnlyDomains, + SymbolChildren, +) +from pybamm.expression_tree.tree_util import _TRANSIENT_SLOTS, layout def run_compatibility_script(source): @@ -25,6 +41,291 @@ def run_compatibility_script(source): assert result.returncode == 0, result.stdout + result.stderr +# writes to attributes named like symbol slots whose receiver is not a symbol +_NON_SYMBOL_WRITES = { + ("pybamm/solvers/idaklu_solver.py", "sol.y"), + ("pybamm/spatial_methods/scikit_finite_element_3d.py", "M._shape"), + ( + ( + "packages/pybamm/tests/unit/test_spatial_methods/" + "test_finite_volume_unstructured.py" + ), + "method._mesh", + ), +} + + +def _written_attributes(node: ast.AST) -> list[tuple[ast.expr, str]]: + """The ``(receiver, attribute)`` pairs written by an assignment-like node.""" + if isinstance(node, ast.Assign | ast.Delete): + targets = list(node.targets) + elif isinstance(node, ast.AugAssign | ast.AnnAssign): + targets = [node.target] + elif ( + isinstance(node, ast.Call) + and ast.unparse(node.func) in ("setattr", "object.__setattr__") + and len(node.args) >= 2 + and isinstance(node.args[1], ast.Constant) + ): + return [(node.args[0], node.args[1].value)] + else: + return [] + written = [] + while targets: + target = targets.pop() + if isinstance(target, ast.Tuple | ast.List): + targets.extend(target.elts) + elif isinstance(target, ast.Starred): + targets.append(target.value) + elif isinstance(target, ast.Attribute): + written.append((target.value, target.attr)) + return written + + +def _is_warn_mutation(statement: ast.stmt) -> bool: + """Whether ``statement`` is a bare ``_warn_mutation(...)`` call.""" + return ( + isinstance(statement, ast.Expr) + and isinstance(statement.value, ast.Call) + and getattr( + statement.value.func, "attr", getattr(statement.value.func, "id", None) + ) + == "_warn_mutation" + ) + + +def _new_instance_name(statement: ast.stmt) -> str | None: + """The name ``statement`` binds, if it is ``name = .__new__(...)``.""" + if ( + isinstance(statement, ast.Assign) + and len(statement.targets) == 1 + and isinstance(statement.targets[0], ast.Name) + and isinstance(statement.value, ast.Call) + and getattr(statement.value.func, "attr", None) == "__new__" + ): + return statement.targets[0].id + return None + + +class _Scope: + """What a function has established before its current top-level statement.""" + + def __init__(self, function: ast.AST): + self.function = function + self.warned = False + self.created: set[str] = set() + + +class _SlotWriteFinder(ast.NodeVisitor): + """ + Collect writes to symbol slots outside construction. Allowed are a symbol's + writes to its own slots in ``__init__`` and ``__setstate__``, writes to an + instance the function created with ``__new__``, and writes after a top-level + ``_warn_mutation(...)`` call, which raises in the test suite. + """ + + def __init__(self, symbol_classes: set[ast.ClassDef], protected: set[str]): + self.symbol_classes = symbol_classes + self.protected = protected + self.classes: list[ast.ClassDef] = [] + self.scopes: list[_Scope] = [] + self.writes: list[str] = [] + + def visit_ClassDef(self, node): + self.classes.append(node) + self.generic_visit(node) + self.classes.pop() + + def visit_FunctionDef(self, node): + scope = _Scope(node) + self.scopes.append(scope) + for statement in node.body: + self.visit(statement) + # only a warning that always runs first guards the writes after it + scope.warned = scope.warned or _is_warn_mutation(statement) + created = _new_instance_name(statement) + if created is not None: + scope.created.add(created) + self.scopes.pop() + + visit_AsyncFunctionDef = visit_FunctionDef + + def generic_visit(self, node): + for receiver, attribute in _written_attributes(node): + if attribute in self.protected and not self._allowed(receiver): + self.writes.append(f"{ast.unparse(receiver)}.{attribute}") + super().generic_visit(node) + + def _allowed(self, receiver: ast.expr) -> bool: + if not self.scopes: + return False + scope = self.scopes[-1] + if scope.warned: + return True + if isinstance(receiver, ast.Name): + if receiver.id in scope.created: + return True + if receiver.id == "self": + if not self.classes or self.classes[-1] not in self.symbol_classes: + return True + return scope.function.name in ("__init__", "__setstate__") + return False + + +def _symbol_class_nodes( + tree: ast.Module, module: str, symbol_names: set[str] +) -> set[ast.ClassDef]: + """ + The classes ``tree`` defines that are symbols: those named in ``symbol_names`` + and those with a base that is, resolved through the file's imports and through + the other classes it defines. + """ + aliases = {} + for node in ast.walk(tree): + if isinstance(node, ast.Import): + aliases.update( + (alias.asname, alias.name) for alias in node.names if alias.asname + ) + elif isinstance(node, ast.ImportFrom) and node.module and not node.level: + aliases.update( + (alias.asname or alias.name, f"{node.module}.{alias.name}") + for alias in node.names + ) + + def dotted(expr: ast.expr) -> str | None: + if isinstance(expr, ast.Name): + return aliases.get(expr.id, expr.id) + if isinstance(expr, ast.Attribute): + prefix = dotted(expr.value) + return prefix and f"{prefix}.{expr.attr}" + return None + + local: set[str] = set() + + def is_symbol(name: str | None) -> bool: + return name is not None and ( + name in local or name in symbol_names or f"{module}.{name}" in symbol_names + ) + + classes = [node for node in ast.walk(tree) if isinstance(node, ast.ClassDef)] + found: set[ast.ClassDef] = set() + while True: + new = [ + node + for node in classes + if node not in found + and (is_symbol(node.name) or any(is_symbol(dotted(b)) for b in node.bases)) + ] + if not new: + return found + found.update(new) + local.update(node.name for node in new) + + +def _declared_slots(node: ast.ClassDef) -> set[str]: + """The slot names a class body declares in ``__slots__``.""" + for statement in node.body: + if isinstance(statement, ast.Assign) and any( + isinstance(target, ast.Name) and target.id == "__slots__" + for target in statement.targets + ): + value = statement.value + items = value.elts if isinstance(value, ast.Tuple | ast.List) else [value] + return { + item.value + for item in items + if isinstance(item, ast.Constant) and isinstance(item.value, str) + } + return set() + + +@functools.cache +def _symbol_names_and_slots() -> tuple[frozenset[str], frozenset[str]]: + """The dotted names pybamm's symbol classes are reached by, and their slots.""" + classes, stack = set(), [pybamm.Symbol] + while stack: + cls = stack.pop() + classes.add(cls) + stack.extend(cls.__subclasses__()) + names = {f"{cls.__module__}.{cls.__qualname__}" for cls in classes} + names.update( + f"pybamm.{cls.__name__}" + for cls in classes + if getattr(pybamm, cls.__name__, None) is cls + ) + slots = set() + for cls in classes: + leaf_fields, state_fields, _ = layout(cls) + slots.update(leaf_fields, state_fields) + return frozenset(names), frozenset(slots) + + +_UNPROTECTED_SLOTS = _TRANSIENT_SLOTS | {"_print_name", "_raw_print_name"} + + +def _slot_writes(tree: ast.Module, module: str = "") -> list[str]: + """The writes in ``tree`` to symbol slots outside construction, as + ``receiver.attribute``. Symbol classes are resolved from the source, so the + slots declared by subclasses defined in ``tree`` are protected too.""" + symbol_names, protected = _symbol_names_and_slots() + symbol_nodes = _symbol_class_nodes(tree, module, symbol_names) + for node in symbol_nodes: + protected |= _declared_slots(node) + finder = _SlotWriteFinder(symbol_nodes, protected - _UNPROTECTED_SLOTS) + finder.visit(tree) + return finder.writes + + +def _notebook_tree(path: Path) -> ast.Module: + """The code cells of a notebook that parse as Python, without IPython magics.""" + body = [] + for cell in json.loads(path.read_text(encoding="utf-8")).get("cells", []): + lines = "".join(cell.get("source", "")).splitlines() + if cell.get("cell_type") != "code" or (lines and lines[0].startswith("%%")): + continue + code = "\n".join( + line for line in lines if not line.lstrip().startswith(("%", "!")) + ) + try: + body.extend(ast.parse(code).body) + except SyntaxError: + continue + return ast.Module(body=body, type_ignores=[]) + + +def _pybamm_code(): + """``(path, module, tree)`` for PyBaMM's own Python: the package, then the + tests, examples and docs of the repository, notebooks included.""" + package = Path(pybamm.__file__).parent + for path in sorted(package.rglob("*.py")): + relative = path.relative_to(package.parent).as_posix() + module = relative.removesuffix(".py").replace("/", ".") + tree = ast.parse(path.read_text(encoding="utf-8")) + yield relative, module.removesuffix(".__init__"), tree + repository = Path(__file__).parents[5] + for directory in ("packages/pybamm/tests", "examples", "docs"): + for path in sorted((repository / directory).rglob("*")): + if {"_build", ".ipynb_checkpoints"} & set(path.parts): + continue + if path.suffix == ".py": + tree = ast.parse(path.read_text(encoding="utf-8")) + elif path.suffix == ".ipynb": + tree = _notebook_tree(path) + else: + continue + yield path.relative_to(repository).as_posix(), "", tree + + +def _post_construction_slot_writes() -> set[tuple[str, str]]: + """Every ``(path, receiver.attribute)`` write in PyBaMM's code that targets a + symbol slot outside construction.""" + return { + (path, write) + for path, module, tree in _pybamm_code() + for write in _slot_writes(tree, module) + } + + class TestLegacyMutation: @pytest.mark.parametrize( "action", @@ -40,10 +341,6 @@ class TestLegacyMutation: lambda symbol: symbol.clear_domains(), lambda symbol: symbol.copy_domains(pybamm.Symbol("other")), lambda symbol: symbol.set_id(), - lambda symbol: symbol.domain.append("separator"), - lambda symbol: symbol.domains.update(primary=["separator"]), - lambda symbol: symbol.domains.__setitem__("primary", ["separator"]), - lambda symbol: symbol.domains["primary"].clear(), ], ) def test_suite_forbids_mutation_even_without_debug_or_warnings(self, action): @@ -69,10 +366,207 @@ def test_suite_forbids_subclass_setters(self): assert scalar.value == 1 assert parameter.input_names == ["x"] + def test_suite_forbids_construction_helper_setters(self): + pybamm.settings.debug_mode = False + vector = pybamm.Vector([1, 2]) + data = np.linspace(0, 1, 3) + interpolant = pybamm.Interpolant(data, data, pybamm.t) + state_vector = pybamm.StateVector(slice(0, 2)) + entries_string = (vector.entries_string, interpolant.entries_string) + with pytest.raises(AttributeError, match=r"entries_string"): + vector.entries_string = ("changed",) + with pytest.raises(AttributeError, match=r"entries_string"): + interpolant.entries_string = "changed" + with pytest.raises(AttributeError, match=r"set_evaluation_array"): + state_vector.set_evaluation_array([slice(0, 1)], None) + assert (vector.entries_string, interpolant.entries_string) == entries_string + assert state_vector.evaluation_array == [True, True] + + def test_accessors_return_immutable_containers(self): + child = pybamm.Variable("a", domain="negative electrode") + binary = pybamm.Addition(child, pybamm.Scalar(2)) + parameter = pybamm.FunctionParameter("f", {"x": child}) + # immutable sequences are returned as stored + assert binary.children is binary.children is binary.orphans + assert type(binary.children) is SymbolChildren + assert child.domain is child._domains["primary"] + assert type(child.secondary_domain) is DomainNames + assert parameter.input_names is parameter.input_names + assert type(parameter.input_names) is InputNames + # the shared domains mapping is only ever handed out as a copy + assert type(child.domains) is ReadOnlyDomains + assert child.domains == child._domains + assert child.domains is not child._domains + assert child.domain == ["negative electrode"] + assert parameter.input_names == ["x"] + + def test_immutable_sequences_behave_like_lists(self): + child = pybamm.Variable("a", domain=["negative electrode", "separator"]) + binary = pybamm.Addition(child, pybamm.Scalar(2)) + names = child.domain + assert names == ["negative electrode", "separator"] == names + assert operator.ne(names, ["separator"]) is True + assert operator.ne(names, list(names)) is False + assert operator.add(names, ["x"]) == ["negative electrode", "separator", "x"] + assert operator.add(["x"], names) == ["x", "negative electrode", "separator"] + assert names * 2 == list(names) * 2 + assert repr(names) == repr(list(names)) == str(names) + assert type(names.copy()) is list and names.copy() == names + assert binary.children == [child, pybamm.Scalar(2)] + assert json.loads(json.dumps(names)) == names + for sequence in (names, binary.children, binary.children[0].domains): + assert pickle.loads(pickle.dumps(sequence)) == sequence # nosec B301 + assert copy.deepcopy(sequence) == sequence + assert type(copy.copy(sequence)) is type(sequence) + + def test_base_class_methods_cannot_change_symbols(self): + def symbols(): + first = pybamm.Variable("a", domain="negative electrode") + second = pybamm.Variable("b", domain="negative electrode") + binary = pybamm.Addition(first, pybamm.Scalar(2)) + parameter = pybamm.FunctionParameter("f", {"x": first}) + return first, second, binary, parameter + + edits = [ + lambda a, b, e, f: list.__setitem__(e.children, 0, pybamm.Scalar(9)), + lambda a, b, e, f: heapq.heappush(e.children, pybamm.Scalar(9)), + lambda a, b, e, f: list.append(a.domain, "separator"), + lambda a, b, e, f: list.append(f.input_names, "y"), + lambda a, b, e, f: dict.__setitem__(a.domains, "primary", ["separator"]), + lambda a, b, e, f: dict.clear(a.to_json()["domains"]), + lambda a, b, e, f: dict.clear(e.get_children_domains([a, b])), + lambda a, b, e, f: dict.clear(a.read_domain_or_domains(None, None, None)), + ] + for edit in edits: + first, second, binary, parameter = symbols() + identities = [s.id for s in (first, second, binary, parameter)] + try: + edit(first, second, binary, parameter) + except TypeError: + pass # tuples have no storage to edit + assert [s.id for s in (first, second, binary, parameter)] == identities + assert binary.children == [first, pybamm.Scalar(2)] + assert first.domain == second.domain == ["negative electrode"] + assert parameter.input_names == ["x"] + # the interned domains every new symbol shares are untouched + assert pybamm.Variable("c", domain="negative electrode").domain == [ + "negative electrode" + ] + assert pybamm.Variable("d").domain == [] + + def test_container_edits_raise_and_leave_symbols_unchanged(self): + child = pybamm.Variable("a", domain="negative electrode") + equal = pybamm.Variable("a", domain="negative electrode") + binary = pybamm.Addition(child, pybamm.Scalar(2)) + parameter = pybamm.FunctionParameter("f", {"x": child}) + edits = { + r"children are immutable": [ + lambda: binary.children.__setitem__(0, child), + lambda: binary.children.append(child), + lambda: binary.orphans.clear(), + ], + r"domains are immutable": [ + lambda: child.domain.append("separator"), + lambda: child.domain.__iadd__(["separator"]), + lambda: child.domain.__imul__(2), + lambda: child.secondary_domain.extend(["current collector"]), + lambda: child.domains.update(primary=["separator"]), + lambda: child.domains.__setitem__("primary", ["separator"]), + lambda: child.domains.__ior__({"primary": ["separator"]}), + lambda: child.domains.pop("primary"), + lambda: child.domains["primary"].clear(), + ], + r"input names are immutable": [ + lambda: parameter.input_names.append("y"), + lambda: parameter.input_names.__setitem__(0, "z"), + ], + } + symbols = (child, equal, binary, parameter) + identities = [symbol.id for symbol in symbols] + for message, actions in edits.items(): + for action in actions: + with pytest.raises(TypeError, match=message): + action() + assert [symbol.id for symbol in symbols] == identities + assert binary.children == [child, pybamm.Scalar(2)] + assert child.domain == equal.domain == ["negative electrode"] + assert parameter.input_names == ["x"] + + def test_unpickled_plain_lists_become_immutable(self): + parameter = pybamm.FunctionParameter("f", {"x": pybamm.Variable("a")}) + state = parameter.__getstate__() + state["_children"] = list(state["_children"]) + state["_input_names"] = list(state["_input_names"]) + restored = object.__new__(pybamm.FunctionParameter) + restored.__setstate__(state) + assert type(restored.children) is SymbolChildren + assert type(restored.input_names) is InputNames + assert restored == parameter + + def test_symbol_attribute_writes_are_not_intercepted(self): + # a write hook taxes every slot a constructor sets, doubling construction + # time; direct slot writes are rejected statically instead + assert pybamm.Symbol.__setattr__ is object.__setattr__ + + def test_pybamm_code_never_writes_symbol_slots_after_construction(self): + assert _post_construction_slot_writes() == _NON_SYMBOL_WRITES + + def test_slot_write_finder_only_allows_construction_and_guarded_writes(self): + # scanned like a test or example file: symbol classes come from the source + source = textwrap.dedent( + """ + import pybamm + from pybamm import Scalar as ImportedScalar + + class Scalar(pybamm.Scalar): + def __init__(self, value): + super().__init__(value) + self._value = value + def deprecated_setter(self, value): + _warn_mutation("value", "") + self._value = value + def writes_before_warning(self, value): + self._value = value + _warn_mutation("value", "") + def warns_conditionally(self, value, flag): + if flag: + _warn_mutation("value", "") + self._value = value + @classmethod + def _from_json(cls, snippet, existing): + instance = cls.__new__(cls) + instance._value = snippet + existing._value = snippet + return instance + class Subclass(Scalar): + def bump(self): + self._value = 2 + class Aliased(ImportedScalar): + __slots__ = ("_extra",) + def bump(self): + self._extra = 1 + def discretise(symbol, vector): + symbol._value = 2 + vector[symbol._value] = 1 + class Helper: + def update(self): + self._value = 3 + """ + ) + assert _slot_writes(ast.parse(source)) == [ + "self._value", + "self._value", + "existing._value", + "self._value", + "self._extra", + "symbol._value", + ] + def test_deprecated_setters_and_methods(self): run_compatibility_script( """ import warnings + import numpy as np import pybamm from pybamm.expression_tree.legacy_mutation import SymbolMutationDeprecationWarning @@ -122,6 +616,16 @@ def mutate(action): parameter = pybamm.FunctionParameter("f", {"x": scalar}) mutate(lambda: setattr(parameter, "input_names", ["y"])) assert parameter.input_names == ["y"] + vector = pybamm.Vector([1, 2]) + mutate(lambda: setattr(vector, "entries_string", ("changed",))) + assert vector.entries_string == ("changed",) + data = np.linspace(0, 1, 3) + interpolant = pybamm.Interpolant(data, data, pybamm.t) + mutate(lambda: setattr(interpolant, "entries_string", "changed")) + assert interpolant.entries_string == "changed" + state_vector = pybamm.StateVector(slice(0, 2)) + mutate(lambda: state_vector.set_evaluation_array([slice(0, 1)], None)) + assert state_vector.evaluation_array == [True] """ ) @@ -157,7 +661,7 @@ def test_mutation_invalidates_shared_parents_and_shape_caches(self): """ ) - def test_nested_domain_mutation_does_not_modify_equal_symbols(self): + def test_domain_setter_does_not_modify_equal_symbols(self): run_compatibility_script( """ import warnings @@ -168,15 +672,9 @@ def test_nested_domain_mutation_does_not_modify_equal_symbols(self): first = pybamm.Variable("a", domain="negative electrode") second = pybamm.Variable("a", domain="negative electrode") second_id = second.id - alias = first.domain + assert first._domains is second._domains with warnings.catch_warnings(record=True) as caught: - alias.append("separator") - assert caught - assert first.domain == ["negative electrode", "separator"] - assert second.domain == ["negative electrode"] - assert second.id == second_id - with warnings.catch_warnings(record=True) as caught: - first.domains["primary"] = ["positive electrode"] + first.domains = {"primary": ["positive electrode"]} assert caught assert first.domain == ["positive electrode"] assert second.domain == ["negative electrode"] @@ -186,22 +684,15 @@ def test_nested_domain_mutation_does_not_modify_equal_symbols(self): """ ) - def test_suite_forbids_child_and_input_list_mutation(self): + def test_suite_forbids_child_setters(self): pybamm.settings.debug_mode = False child = pybamm.Variable("a") binary = pybamm.Addition(child, pybamm.Scalar(2)) unary = pybamm.Negate(child) - parameter = pybamm.FunctionParameter("f", {"x": child}) actions = [ lambda: setattr(binary, "left", object()), lambda: setattr(binary, "right", object()), lambda: setattr(unary, "child", object()), - lambda: binary.children.__setitem__(0, child), - lambda: binary.orphans.clear(), - lambda: parameter.input_names.append("y"), - lambda: child.domains.__ior__({"primary": ["separator"]}), - lambda: child.domain.__iadd__(["separator"]), - lambda: child.domain.__imul__(2), ] with warnings.catch_warnings(): warnings.simplefilter("ignore") @@ -210,9 +701,8 @@ def test_suite_forbids_child_and_input_list_mutation(self): action() assert binary.left is unary.child is child assert binary.right == pybamm.Scalar(2) - assert parameter.input_names == ["x"] - def test_child_and_input_list_compatibility(self): + def test_child_setter_compatibility(self): run_compatibility_script( """ import warnings @@ -235,139 +725,8 @@ def test_child_and_input_list_compatibility(self): assert binary.right == pybamm.Scalar(3) assert parent.id != old_id assert parent == pybamm.Negate(pybamm.Addition(replacement, pybamm.Scalar(3))) - children = binary.children - orphans = binary.orphans - with warnings.catch_warnings(record=True) as caught: - children[0] = child - orphans[1] = pybamm.Scalar(4) - assert len(caught) == 2 - assert binary.left is child - assert binary.right == pybamm.Scalar(4) - assert children == orphans == binary.children - parameter = pybamm.FunctionParameter("f", {"x": child}) - names = parameter.input_names - with warnings.catch_warnings(record=True) as caught: - parameter.input_names += ["y"] - names[0] = "z" - parameter.input_names *= 2 - assert caught - assert parameter.input_names == ["z", "y", "z", "y"] - """ - ) - - def test_retained_domain_views_and_mutation_return_values(self): - run_compatibility_script( - """ - import warnings - import pybamm - from pybamm.expression_tree.legacy_mutation import SymbolMutationDeprecationWarning - - warnings.simplefilter("always", SymbolMutationDeprecationWarning) - symbol = pybamm.Variable("a", domain="negative electrode") - mapping = symbol.domains - first = symbol.domain - second = symbol.domain - with warnings.catch_warnings(record=True) as caught: - first.append("separator") - second.append("positive electrode") - mapping["secondary"] = ["current collector"] - assert len(caught) == 3 - expected = ["negative electrode", "separator", "positive electrode"] - assert first == second == symbol.domain == expected - assert mapping["primary"] == expected - assert symbol.domains["secondary"] == ["current collector"] - with warnings.catch_warnings(record=True) as caught: - symbol.domains |= {"tertiary": ["negative particle"]} - symbol.domains["primary"] += ["positive particle"] - symbol.domains["primary"] *= 1 - removed = first.pop() - existing = mapping.setdefault("secondary", ["unused"]) - assert caught - assert removed == "positive particle" - assert existing == ["current collector"] - assert symbol.domain == expected - assert mapping["tertiary"] == ["negative particle"] - with warnings.catch_warnings(record=True) as caught: - removed = mapping.pop("tertiary") - inserted = mapping.setdefault("tertiary", ["positive particle"]) - missing = mapping.pop("missing", "default") - last_key, last_value = mapping.popitem() - assert len(caught) == 4 - assert removed == ["negative particle"] - assert inserted == ["positive particle"] - assert missing == "default" - assert last_key == "tertiary" - assert last_value == ["positive particle"] - assert "tertiary" not in symbol.domains - with warnings.catch_warnings(record=True) as caught: - symbol.domains.setdefault("tertiary", []).append("negative particle") - assert len(caught) == 2 - assert symbol.domains["tertiary"] == ["negative particle"] - """ - ) - - def test_domain_collection_mutators(self): - run_compatibility_script( - """ - import warnings - import pybamm - from pybamm.expression_tree.legacy_mutation import SymbolMutationDeprecationWarning - - warnings.simplefilter("always", SymbolMutationDeprecationWarning) - cases = [ - (lambda value: value.__setitem__(0, "c"), ["c", "a"], None), - (lambda value: value.__delitem__(0), ["a"], None), - (lambda value: value.extend(["c"]), ["b", "a", "c"], None), - (lambda value: value.insert(1, "c"), ["b", "c", "a"], None), - (lambda value: value.remove("b"), ["a"], None), - (lambda value: value.sort(), ["a", "b"], None), - (lambda value: value.reverse(), ["a", "b"], None), - (lambda value: value.clear(), [], None), - (lambda value: value.pop(0), ["a"], "b"), - ] - for mutate, expected, returned in cases: - symbol = pybamm.Variable("a", domain=["b", "a"]) - with warnings.catch_warnings(record=True) as caught: - result = mutate(symbol.domain) - assert len(caught) == 1 - assert result == returned - assert symbol.domain == expected - symbol = pybamm.Variable("a", domain="negative electrode") - with warnings.catch_warnings(record=True) as caught: - del symbol.domains["quaternary"] - assert len(caught) == 1 - assert "quaternary" not in symbol.domains - with warnings.catch_warnings(record=True) as caught: - result = symbol.domains.clear() - assert len(caught) == 1 - assert result is None - assert symbol.domains == {} - """ - ) - - def test_legacy_domain_reordering_preserves_hash_and_symbol_identity(self): - run_compatibility_script( - """ - import warnings - import pybamm - - first = pybamm.Variable( - "a", - domain="negative electrode", - auxiliary_domains={"secondary": "current collector"}, - ) - second = pybamm.Variable( - "a", - domain="negative electrode", - auxiliary_domains={"secondary": "current collector"}, - ) - with warnings.catch_warnings(): - warnings.simplefilter("ignore") - primary = first.domains.pop("primary") - first.domains["primary"] = primary - assert first.domains == second.domains - assert hash(first._domains) == hash(second._domains) - assert first.id == second.id + assert type(binary.children).__name__ == "SymbolChildren" + assert type(unary.children).__name__ == "SymbolChildren" """ ) diff --git a/packages/pybamm/tests/unit/test_expression_tree/test_symbol.py b/packages/pybamm/tests/unit/test_expression_tree/test_symbol.py index b3abc9394c..73008acc30 100644 --- a/packages/pybamm/tests/unit/test_expression_tree/test_symbol.py +++ b/packages/pybamm/tests/unit/test_expression_tree/test_symbol.py @@ -12,12 +12,14 @@ import pybamm from pybamm.expression_tree.binary_operators import _Heaviside +from pybamm.expression_tree.operations.serialise_kernel import decode, encode from pybamm.expression_tree.symbol import domain_size class TestDomainSize: def test_empty_domain(self): assert domain_size([]) == 1 + assert domain_size(()) == 1 assert domain_size(None) == 1 def test_fixed_domains(self): @@ -600,8 +602,6 @@ def build(i): with ThreadPoolExecutor(8) as executor: symbols = list(executor.map(build, range(200))) assert [symbol.scale.value for symbol in symbols] == list(range(1, 201)) - with pytest.raises(AttributeError, match=r"immutable once constructed"): - symbols[0]._name = "changed" def test_edge_direction_is_part_of_identity(self): a = pybamm.Variable("a", domain="negative electrode") @@ -654,6 +654,28 @@ def test_with_domains_validates_domain_mapping(self, domains): with pytest.raises(pybamm.DomainError): pybamm.Variable("a").with_domains(domains) + @pytest.mark.parametrize( + "rebuild", + [ + lambda domains: {level: names[:] for level, names in domains.items()}, + lambda domains: {level: tuple(names) for level, names in domains.items()}, + lambda domains: decode(encode(domains)), + ], + ids=["slices", "tuples", "serialise kernel"], + ) + def test_domains_accept_any_sequence_of_names(self, rebuild): + # names no other test uses, so validation cannot be skipped by its cache + a = pybamm.Variable( + "a", + domain="sequence test a", + auxiliary_domains={"secondary": "sequence test b"}, + ) + domains = rebuild(a.domains) + assert pybamm.Variable("a", domains=domains) == a + assert a.without_domains().with_domains(domains) == a + shape = pybamm.evaluate_for_shape_using_domain(domains).shape + assert shape == a.shape_for_testing + def test_domain_interning_is_not_public(self): assert not hasattr(pybamm, "intern_domains")