Skip to content
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

Merged
merged 31 commits into from Mar 18, 2020
Merged

Issue 858 ddt #888

merged 31 commits into from Mar 18, 2020

Conversation

martinjrobins
Copy link
Contributor

@martinjrobins martinjrobins commented Mar 14, 2020

Description

Fixes #858.

Changes:

  • a new Symbol class pybamm.VariableDot that represents the derivative of a state variable wrt time, the time derivative of a pybamm.Variable results in a pybamm.VariableDot
  • a new Symbol class pybamm.StateVariableDot that represents the state time derivative vector, the time derivative of a pybamm.StateVector results in a pybamm.StateVectorDot
    discretisation of a pybamm.VariableDot results in a corresponding pybamm.StateVectorDot
  • change Symbol.evaluate to also take ydot, the derivative of the state vector, as an argument
  • raise error in the base_model checks if a pybamm.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.

  • New feature (non-breaking change which adds functionality)

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
Copy link

codecov bot commented Mar 15, 2020

Codecov Report

Merging #888 into develop will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           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.

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

Copy link
Member

@valentinsulzer valentinsulzer left a 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]
Copy link
Member

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)

Copy link
Contributor Author

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.

Copy link
Member

@valentinsulzer valentinsulzer left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@martinjrobins martinjrobins merged commit 37adde2 into develop Mar 18, 2020
@martinjrobins martinjrobins deleted the issue-858-ddt branch March 18, 2020 13:39
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.

time derivative operator
2 participants