-
-
Notifications
You must be signed in to change notification settings - Fork 534
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue 858 ddt #888
Issue 858 ddt #888
Conversation
…rors if taking time derivative of *Dot classes
Codecov Report
@@ Coverage Diff @@
## develop #888 +/- ##
========================================
Coverage 97.97% 97.97%
========================================
Files 214 214
Lines 11281 11281
========================================
Hits 11052 11052
Misses 229 229 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code all looks great and thanks for catching those bugs as well!
Can you address the missing coverage?
@@ -96,10 +96,10 @@ def initialise_1D(self): | |||
inputs = {name: inp[idx] for name, inp in self.inputs.items()} | |||
if self.known_evals: | |||
entries[idx], self.known_evals[t] = self.base_variable.evaluate( | |||
t, u, inputs, known_evals=self.known_evals[t] | |||
t, u, u=inputs, known_evals=self.known_evals[t] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first u
looks especially dumb now here 🙈 (my mistake changing it a while back). But makes me wonder if we should use p=
for inputs/parameters instead of u=
(not for this issue)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It certainly shouldn't be u
, which is not very explanatory. p
is better, although params' or
parameters' would be even more obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks!
Description
Fixes #858.
Changes:
pybamm.VariableDot
that represents the derivative of a state variable wrt time, the time derivative of apybamm.Variable
results in apybamm.VariableDot
pybamm.StateVariableDot
that represents the state time derivative vector, the time derivative of apybamm.StateVector
results in apybamm.StateVectorDot
discretisation of a
pybamm.VariableDot
results in a correspondingpybamm.StateVectorDot
Symbol.evaluate
to also take ydot, the derivative of the state vector, as an argumentpybamm.VariableDot
exists in any rhs or algebraic equations (i.e. equations should be in semi-explicit form)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 docs
and then$ make clean; make html
You can run all three at once, using
$ python run-tests.py --quick
.Further checks: