Issue 937 sensitivity #940
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
valentinsulzer
requested review from
martinjrobins,
brosaplanella and
TomTranter
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
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 |
|
Then it looks good to me :) |
But the issue is that I don't really know how to test it |
| @@ -19,7 +19,7 @@ Positive tab centre z-coordinate [m],0.114,Tab at top, | |||
| # Electrical,,, | |||
| Cell capacity [A.h],17,Manufacturer, | |||
| Typical current [A],1,, | |||
|
|
|||
| Current function [A],1,default current function, | |||
There was a problem hiding this comment.
are these changes to that parameters supposed to be in this PR?
| # Make sure typical current is non-zero | ||
| if "Typical current [A]" in values and values["Typical current [A]"] == 0: | ||
| raise ValueError( | ||
| "'Typical current [A]' cannot be zero. A possible alternative is to " | ||
| "set 'Current function [A]' to `0` instead." | ||
| ) | ||
| if "C-rate" in values and "Current function [A]" in values: | ||
| if "C-rate" in values: |
There was a problem hiding this comment.
are these supposed to be in this PR, doesn't seem like these are anything to do with sensitivities?
|
I merged in another branch which was about to be merged to develop, thought those changes would disappear once that branch got merged |
Description
Added sensitivity to
CasadiAlgebraicSolverclass. To do this, added aCasadiSolutionandProcessedCasadiVariableclass, which can hold a symbolic solution (i.e. inputs are symbolic, not yet provided). The value and sensitivity of aProcessedCasadiVariablecan 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.
Key checklist:
$ flake8$ python run-tests.py --unit$ cd docsand then$ make clean; make htmlYou can run all three at once, using
$ python run-tests.py --quick.Further checks: