Issue 685 casadi interpolation #714

valentinsulzer · 2019-11-07T17:55:36Z

Description

Set up interpolants in casadi form
Changed defaults to using the casadi structure for jacobians etc
Added casadi-safe-solver as a fallback for when SUNDIALS solvers aren't installed.

Fixes #685

Should allow fixing #463 by testing examples both with and without the optional solvers (requires changing scikits dae solver example to casadi, or skipping).

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.

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

…ssue-685-casadi-interpolation

codecov · 2019-11-07T23:25:32Z

Codecov Report

Merging #714 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #714      +/-   ##
==========================================
+ Coverage   98.17%   98.18%   +0.01%     
==========================================
  Files         176      175       -1     
  Lines        9135     9089      -46     
==========================================
- Hits         8968     8924      -44     
+ Misses        167      165       -2

Impacted Files	Coverage Δ
pybamm/solvers/casadi_solver.py	`98.7% <ø> (ø)`	⬆️
...bamm/models/full_battery_models/lithium_ion/dfn.py	`100% <ø> (ø)`	⬆️
...m/models/full_battery_models/base_battery_model.py	`99.62% <ø> (-0.01%)`	⬇️
pybamm/models/submodels/electrode/ohm/full_ohm.py	`100% <ø> (+1.78%)`	⬆️
...ybamm/models/full_battery_models/lead_acid/loqs.py	`100% <ø> (ø)`	⬆️
...ybamm/models/full_battery_models/lead_acid/full.py	`100% <ø> (ø)`	⬆️
...models/submodels/electrode/ohm/surface_form_ohm.py	`100% <ø> (+3.44%)`	⬆️
...bamm/models/full_battery_models/lithium_ion/spm.py	`100% <ø> (ø)`	⬆️
...bamm/models/submodels/electrode/ohm/leading_ohm.py	`100% <ø> (+3.03%)`	⬆️
...l/conductivity/full_stefan_maxwell_conductivity.py	`100% <ø> (+3.33%)`	⬆️
... and 20 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 aba1aae...234e7e8. Read the comment docs.

rtimms

looks great, thanks @tinosulzer !

rtimms · 2019-11-08T09:58:52Z

examples/notebooks/change-input-current.ipynb

@@ -22,7 +22,7 @@
    "\n",
    "In this notebook we will use the SPM as the example model, and change the input current from the default option. If you are not familiar with running a model in PyBaMM, please see [this](./models/SPM.ipynb) notebook for more details.\n",
    "\n",
-    "In PyBaMM, the current function is set using the parameter \"Current function\". By default this is set to be a constant current provided by the class [`pybamm.GetConstantCurrent`](https://pybamm.readthedocs.io/en/latest/source/parameters/standard_current_functions/get_constant_current.html).  This class takes a single optional argument \"current\" which is the size of the current in Amperes. If no argument is passed, the value of the current is set by the parameter \"Typical current [A]\" (see [this](parameter-values.ipynb) notebook for more information about parameters).\n",
+    "In PyBaMM, the current function is set using the parameter \"Current function\". By default this is set to be a constant current provided by the class [`pybamm.ConstantCurrent`](https://pybamm.readthedocs.io/en/latest/source/parameters/standard_current_functions/get_constant_current.html).  This class takes a single optional argument \"current\" which is the size of the current in Amperes. If no argument is passed, the value of the current is set by the parameter \"Typical current [A]\" (see [this](parameter-values.ipynb) notebook for more information about parameters).\n",


should the read the docs link now be constant_current instead of get_constant_current?

Just looking at it now I can't find the page this was supposed to link to anyway. Getting rid of it in #699 so just going to leave it for now. We should look into finding a way to test hyperlinks in markdown/notebooks to make sure they exist

rtimms · 2019-11-08T10:00:19Z

examples/notebooks/change-input-current.ipynb

@@ -240,14 +212,14 @@
   "source": [
    "## Adding your own current function <a name=\"function\"></a>\n",
    "\n",
-    "A user defined current function can be passed to any model using the class [`pybamm.GetUserCurrent`](https://pybamm.readthedocs.io/en/latest/source/parameters/standard_current_functions/get_user_current). The class takes in a method, which returns the current as a function of time, followed by any keyword arguments required by the method.  \n",
+    "A user defined current function can be passed to any model using the class [`pybamm.UserCurrent`](https://pybamm.readthedocs.io/en/latest/source/parameters/standard_current_functions/get_user_current). The class takes in a method, which returns the current as a function of time, followed by any keyword arguments required by the method.  \n",


same re read the docs link here

rtimms · 2019-11-08T10:02:07Z

pybamm/models/base_model.py

@@ -479,3 +479,15 @@ def check_variables(self):
                        var
                    )
                )
+
+    @property


rtimms · 2019-11-08T10:02:58Z

pybamm/models/full_battery_models/lead_acid/base_lead_acid_model.py

@@ -34,8 +34,22 @@ def default_geometry(self):

    @property
    def default_var_pts(self):
+        # Choose points that give uniform grid for the standard parameter values


makes sense. is this the case for li-ion too?

Thought I needed this to make one of the tests work but it turned out to be an issue with the solver. I guess we could add functionality so that the user can pass in grid points for, say, x_n, and then "uniform" for the other two, and it automatically selects the right number of points to make the grid uniform?

valentinsulzer and others added 17 commits November 4, 2019 23:07

#685 set up casadi interpolation

764ff2e

Merge branch 'master' into issue-685-casadi-interpolation

0f6fc95

#685 reformat current data

e0a4661

#685 change defaults to IDAKLU or casadi

57404c3

#685 remove skips from tests as they should use casadi

53c0b56

#685 move default solver to base model

f632714

#685 update (remove) solver tests

cc33c95

#685 remove some slow tests

52e09a8

#685 get drive cycle example working

1175cbf

#685 fix casadi tests

9911d41

#685 fix interpolation example

bc31cd9

#685 fix integration tests

bbc9bce

Merge branch 'master' into issue-685-casadi-interpolation

db4f643

#715 update sundials to 4.1.0

0029b95

#685 update changelog

6ffcf81

Merge remote-tracking branch 'origin/issue-715-scikit-install' into i…

edf18a4

…ssue-685-casadi-interpolation

#685 update solver tests

8829213

#685 coverage

234e7e8

valentinsulzer requested review from rtimms and Scottmar93 November 8, 2019 03:04

rtimms approved these changes Nov 8, 2019

View reviewed changes

valentinsulzer merged commit 7d31c68 into master Nov 8, 2019

valentinsulzer deleted the issue-685-casadi-interpolation branch November 8, 2019 13:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 685 casadi interpolation #714

Issue 685 casadi interpolation #714

valentinsulzer commented Nov 7, 2019 •

edited

Loading

codecov bot commented Nov 7, 2019 •

edited

Loading

rtimms left a comment

rtimms Nov 8, 2019

valentinsulzer Nov 8, 2019

rtimms Nov 8, 2019

rtimms Nov 8, 2019

rtimms Nov 8, 2019

valentinsulzer Nov 8, 2019

Issue 685 casadi interpolation #714

Issue 685 casadi interpolation #714

Conversation

valentinsulzer commented Nov 7, 2019 • edited Loading

Description

Type of change

Key checklist:

Further checks:

codecov bot commented Nov 7, 2019 • edited Loading

Codecov Report

rtimms left a comment

Choose a reason for hiding this comment

rtimms Nov 8, 2019

Choose a reason for hiding this comment

valentinsulzer Nov 8, 2019

Choose a reason for hiding this comment

rtimms Nov 8, 2019

Choose a reason for hiding this comment

rtimms Nov 8, 2019

Choose a reason for hiding this comment

rtimms Nov 8, 2019

Choose a reason for hiding this comment

valentinsulzer Nov 8, 2019

Choose a reason for hiding this comment

valentinsulzer commented Nov 7, 2019 •

edited

Loading

codecov bot commented Nov 7, 2019 •

edited

Loading