Generalised symbol copying #4023
Conversation
Abstract out checks for if new children are provided Improve _unary_new_copy doc strings
This comment was marked as outdated.
This comment was marked as outdated.
…tion Allows for classes with different input parameters to be called without re-defining the whole create_copy function (with unary errors) each time
pipliggins
requested review from
brosaplanella,
DrSOKane,
rtimms,
TomTranter,
kratman and
martinjrobins
as code owners
There was a problem hiding this comment.
thanks @pipliggins, this looks great. I had a quick look through and had a couple of questions, see below. Also I was thinking it might be good to put in an integration test that copies a larger expression tree, e.g. the rhs of the DFN model, and evaluates it a couple of times to check the output is the same?
| @@ -874,7 +874,7 @@ def _process_symbol(self, symbol): | |||
|
|
|||
| elif isinstance(symbol, pybamm.Function): | |||
| disc_children = [self.process_symbol(child) for child in symbol.children] | |||
| return symbol._function_new_copy(disc_children) | |||
| return symbol.new_copy(disc_children) | |||
There was a problem hiding this comment.
can this new_copy replace the _unary_new_copy and _binary_new_copy calls above?
pybamm/expression_tree/averages.py
Outdated
| return x_average(child) | ||
| else: | ||
| raise NotImplementedError( | ||
| f"{self.__class__.__name__} should always be copied using " |
There was a problem hiding this comment.
why do we have to use simplification checks here?
There was a problem hiding this comment.
and others below....
| children = [child.create_copy() for child in self.children] | ||
| return children | ||
|
|
||
| def create_copy( |
There was a problem hiding this comment.
what is the difference between create_copy and new_copy? The docs say that new_copy adds attributes, but not sure what this means?
There was a problem hiding this comment.
new_copy used the print_name setter to ensure the print name was run through the pretty-print functions. No tests broke when I took this out, so I've deprecated new_copy in favour of just using create_copy() (to avoid confusion with model.new_copy())
Description
Provides a general
new_copy()function for pybamm symbols which can accept new children.Some symbols are copied within pybamm using private functions like
_unary_new_copy, usually to use overloaded or convenience functions to perform simplifications before creating a new symbol.As this behaviour can sometimes result in a different symbol class to the one originally copied when new children are provided, this can be overridden using the
perform_simplificationsargument.Fixes #3798
Type of change
Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.
Key checklist:
$ pre-commit run(or$ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)$ python run-tests.py --all(or$ nox -s tests)$ python run-tests.py --doctest(or$ nox -s doctests)You can run integration tests, unit tests, and doctests together at once, using
$ python run-tests.py --quick(or$ nox -s quick).Further checks: