Change "u=" to "param=" and make inputs more explicit #905

valentinsulzer · 2020-03-21T16:40:52Z

Description

Changed u to params when evaluating
Made inputs more explicit in the solvers

Fixes #899

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.

New feature (non-breaking change which adds functionality)
Optimization (back-end change that speeds up the code)
Bug fix (non-breaking change which fixes an issue)

Key checklist:

No style issues: $ flake8
All tests pass: $ python run-tests.py --unit
The documentation builds: $ cd docs and then $ make clean; make html

You can run all three at once, using $ python run-tests.py --quick.

Further checks:

Code is commented, particularly in hard-to-understand areas
Tests added that prove fix is effective or that feature works

…9-u-to-param

codecov · 2020-03-22T19:38:42Z

Codecov Report

Merging #905 into develop will increase coverage by 0.00%.
The diff coverage is 98.59%.

@@           Coverage Diff            @@
##           develop     #905   +/-   ##
========================================
  Coverage    98.00%   98.00%           
========================================
  Files          207      207           
  Lines        10914    10915    +1     
========================================
+ Hits         10696    10697    +1     
  Misses         218      218

Impacted Files	Coverage Δ
pybamm/discretisations/discretisation.py	`99.76% <ø> (ø)`
pybamm/models/event.py	`90.47% <50.00%> (ø)`
pybamm/solvers/scikits_ode_solver.py	`87.32% <88.88%> (+0.55%)`	⬆️
pybamm/expression_tree/array.py	`100.00% <100.00%> (ø)`
pybamm/expression_tree/binary_operators.py	`94.03% <100.00%> (ø)`
pybamm/expression_tree/concatenations.py	`97.98% <100.00%> (ø)`
pybamm/expression_tree/functions.py	`100.00% <100.00%> (ø)`
pybamm/expression_tree/independent_variable.py	`100.00% <100.00%> (ø)`
pybamm/expression_tree/input_parameter.py	`100.00% <100.00%> (ø)`
...mm/expression_tree/operations/convert_to_casadi.py	`93.24% <100.00%> (ø)`
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 998b2c6...f47b4b8. Read the comment docs.

rtimms

thanks @tinosulzer looks good to me! is there a reason we call input parameters "inputs" when calling solve, rather than parameters? maybe parameters or input_parameters would be more obvious?

rtimms · 2020-03-26T10:38:15Z

pybamm/expression_tree/operations/convert_to_casadi.py

-        u : dict
+        y_dot : :class:`casadi.MX`
+            A casadi symbol representing time derivatives of state vectors
+        params : dict
            A dictionary of casadi symbols representing inputs


maybe change to "input parameters" rather than "inputs", just to be super clear

rtimms · 2020-03-26T10:38:44Z

pybamm/expression_tree/operations/convert_to_casadi.py

@@ -38,13 +40,13 @@ def convert(self, symbol, t, y, y_dot, u):
            return self._casadi_symbols[symbol.id]
        except KeyError:
            # Change u to empty dictionary if it's None


Change params to...

rtimms · 2020-03-26T10:39:36Z

pybamm/expression_tree/symbol.py

@@ -556,7 +555,7 @@ def evaluate(self, t=None, y=None, y_dot=None, u=None, known_evals=None):
        y_dot : numpy.array, optional
            array with time derivatives of state values to evaluate when solving
            (default None)
-        u : dict, optional
+        params : dict, optional


as above, maybe this should read "input parameters"

rtimms · 2020-03-26T10:39:59Z

pybamm/expression_tree/variable.py

@@ -172,18 +176,18 @@ def _evaluate_for_shape(self):
        """ See :meth:`pybamm.Symbol.evaluate_for_shape_using_domain()` """
        return np.nan * np.ones((self.size, 1))

-    def _base_evaluate(self, t=None, y=None, y_dot=None, u=None):
+    def _base_evaluate(self, t=None, y=None, y_dot=None, params=None):
        # u should be a dictionary


u -> params

rtimms · 2020-03-26T10:41:06Z

pybamm/solvers/base_solver.py

@@ -826,43 +805,36 @@ def __init__(self, function, name, model):
        self._function = function
        if isinstance(function, casadi.Function):
            self.form = "casadi"
-            self.inputs = casadi.DM()
+            # self.inputs = casadi.DM()


can this (and line 811) go?

valentinsulzer · 2020-03-26T13:29:40Z

Yeah I guess we should use either params everywhere or inputs everywhere. Maybe inputs since that is the one already used in the solver before this PR?

rtimms · 2020-03-26T13:53:37Z

yeah, I think as long as it's consistent it's ok. I guess using inputs avoids confusion with users thinking they need to pass all parameters into the solver

valentinsulzer added 4 commits March 20, 2020 15:41

#899 change u= to params=

2c704fa

#899 make inputs more explicit in _integrate

cd20d99

#899 fix unit tests

a702299

#899 fix scipy events bug

cccd781

valentinsulzer requested review from martinjrobins and rtimms March 21, 2020 18:52

valentinsulzer added 2 commits March 22, 2020 15:36

Merge branch 'develop' of github.com:pybamm-team/PyBaMM into issue-89…

86f0add

…9-u-to-param

#899 changelog

0e8fdde

rtimms approved these changes Mar 26, 2020

View reviewed changes

#899 rob comments

f47b4b8

valentinsulzer merged commit 59e10ce into develop Mar 26, 2020

valentinsulzer deleted the issue-899-u-to-param branch March 26, 2020 17:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change "u=" to "param=" and make inputs more explicit #905

Change "u=" to "param=" and make inputs more explicit #905

valentinsulzer commented Mar 21, 2020

codecov bot commented Mar 22, 2020 •

edited

Loading

rtimms left a comment

rtimms Mar 26, 2020

rtimms Mar 26, 2020

rtimms Mar 26, 2020

rtimms Mar 26, 2020

rtimms Mar 26, 2020

valentinsulzer commented Mar 26, 2020

rtimms commented Mar 26, 2020

Change "u=" to "param=" and make inputs more explicit #905

Change "u=" to "param=" and make inputs more explicit #905

Conversation

valentinsulzer commented Mar 21, 2020

Description

Type of change

Key checklist:

Further checks:

codecov bot commented Mar 22, 2020 • edited Loading

Codecov Report

rtimms left a comment

Choose a reason for hiding this comment

rtimms Mar 26, 2020

Choose a reason for hiding this comment

rtimms Mar 26, 2020

Choose a reason for hiding this comment

rtimms Mar 26, 2020

Choose a reason for hiding this comment

rtimms Mar 26, 2020

Choose a reason for hiding this comment

rtimms Mar 26, 2020

Choose a reason for hiding this comment

valentinsulzer commented Mar 26, 2020

rtimms commented Mar 26, 2020

codecov bot commented Mar 22, 2020 •

edited

Loading