Skip to content

Issue 937 sensitivity#940

Merged
valentinsulzer merged 21 commits into
developfrom
issue-937-sensitivity
Apr 14, 2020
Merged

Issue 937 sensitivity#940
valentinsulzer merged 21 commits into
developfrom
issue-937-sensitivity

Conversation

@valentinsulzer

Copy link
Copy Markdown
Member

Description

Added sensitivity to CasadiAlgebraicSolver class. To do this, added a CasadiSolution and ProcessedCasadiVariable class, which can hold a symbolic solution (i.e. inputs are symbolic, not yet provided). The value and sensitivity of a ProcessedCasadiVariable can then be found for a specific input by calling .value(inputs) or .sensitivity(inputs).

I haven't added any examples or much extra docs for this, I think it can wait until we make this a more fleshed out feature. The next step would be to do the CasadiSolver.

Fixes #937

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

@codecov

codecov Bot commented Apr 4, 2020

Copy link
Copy Markdown

Codecov Report

Merging #940 into develop will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #940      +/-   ##
===========================================
+ Coverage    98.06%   98.11%   +0.05%     
===========================================
  Files          220      221       +1     
  Lines        11370    11518     +148     
===========================================
+ Hits         11150    11301     +151     
+ Misses         220      217       -3     
Impacted Files Coverage Δ
...ybamm/expression_tree/operations/unpack_symbols.py 100.00% <ø> (ø)
pybamm/solvers/processed_variable.py 98.52% <ø> (ø)
pybamm/discretisations/discretisation.py 99.77% <100.00%> (+<0.01%) ⬆️
pybamm/expression_tree/input_parameter.py 100.00% <100.00%> (ø)
pybamm/expression_tree/operations/jacobian.py 100.00% <100.00%> (ø)
pybamm/expression_tree/symbol.py 97.44% <100.00%> (ø)
pybamm/models/base_model.py 98.67% <100.00%> (+0.08%) ⬆️
pybamm/parameters/parameter_values.py 99.08% <100.00%> (-0.15%) ⬇️
pybamm/parameters/print_parameters.py 97.14% <100.00%> (+0.17%) ⬆️
pybamm/simulation.py 98.41% <100.00%> (+1.36%) ⬆️
... and 5 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 33d7b87...514d49b. Read the comment docs.

@martinjrobins martinjrobins left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great @tinosulzer, few comments on the api and testing below. very excited to try this out when you get it implemented using an ode solver :)

Comment thread tests/unit/test_solvers/test_casadi_algebraic_solver.py Outdated
Comment thread pybamm/solvers/processed_casadi_variable.py Outdated
Comment thread pybamm/solvers/processed_casadi_variable.py Outdated
Comment thread tests/unit/test_solvers/test_casadi_algebraic_solver.py Outdated
Comment thread tests/unit/test_solvers/test_casadi_algebraic_solver.py Outdated

@TomTranter TomTranter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Tino, I don't really get how to use the sensitivity functionality and how it's better than running solve with input parameters. It needs an example. I've read through the files changed and I see a few examples of how to use it on very basic examples using one liner models in the tests but I think it needs to be explained using the full battery models properly

@brosaplanella brosaplanella left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Tino! Not my area of expertise (either sensitivity of CasADI) but looks really neat. If I understand correctly, CasADI allows to solve the model leaving one parameter unspecified so it can be passed later and the sensitivity wrt that parameter calculated.

However, I agree with @TomTranter that this feature really needs a tutorial notebook, especially with an application to a complex model such SPM(e) or DFN. Otherwise it is quite hard to understand what is going on.

@valentinsulzer

Copy link
Copy Markdown
Member Author

To be honest, the lack of example/documentation was intentional as I want to keep this "semi-private" until we are settled on the API. Also, it currently only works with algebraic-only models, i.e. not any of the battery models

@brosaplanella

Copy link
Copy Markdown
Member

Then it looks good to me :)

@TomTranter

Copy link
Copy Markdown
Contributor

To be honest, the lack of example/documentation was intentional as I want to keep this "semi-private" until we are settled on the API. Also, it currently only works with algebraic-only models, i.e. not any of the battery models

But the issue is that I don't really know how to test it

@martinjrobins martinjrobins left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with these changes, thanks @tinosulzer. There was some stuff in this PR about c-rate, should this be in here?

Cell capacity [A.h],17,Manufacturer,
Typical current [A],1,,

Current function [A],1,default current function,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these changes to that parameters supposed to be in this PR?

"set 'Current function [A]' to `0` instead."
)
if "C-rate" in values and "Current function [A]" in values:
if "C-rate" in values:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these supposed to be in this PR, doesn't seem like these are anything to do with sensitivities?

@valentinsulzer valentinsulzer changed the base branch from develop to master April 14, 2020 13:13
@valentinsulzer valentinsulzer changed the base branch from master to develop April 14, 2020 13:13
@valentinsulzer

Copy link
Copy Markdown
Member Author

I merged in another branch which was about to be merged to develop, thought those changes would disappear once that branch got merged

@valentinsulzer valentinsulzer merged commit 9b72b1e into develop Apr 14, 2020
@valentinsulzer valentinsulzer deleted the issue-937-sensitivity branch April 14, 2020 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add sensitivity to casadi root-finder

4 participants