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

Explicitly integrate variables which appear nowhere else on the rhs #2348

Merged
merged 43 commits into from
Oct 12, 2022

Conversation

aabills
Copy link
Contributor

@aabills aabills commented Oct 6, 2022

Description

This change moves variables from the model which do not appear on the right hand side to the variables dictionary with a pybamm.ExplicitTimeIntegral wrapper. When called by the solution object, the pybamm.ExplicitTimeIntegral integrates the variable (which can be easily explicitly calculated from the solution object).

Variables are only moved if all of the following is true

  • the variable in question does not appear in the calculation of any other variable in the algebraic equations, the rhs, or the boundary conditions
  • the variable does not appear in y_slices (needed for experiments in the case that the variable is used in one experiment step and not others).
  • the variable is 0 dimensional (len(var.domains)==0)
  • the variable has not already been discretized.
  • there is at least one more variable on the rhs

Issue #2240 provides a brief overview of the impetus for the PR, but the main point is that in the case where the model has a rest step, not doing this results in a singular Jacobian matrix (due to Discharge capacity [A.h], which can cause slowdowns and failures in some solvers.

Fixes #2240

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

@aabills aabills changed the title Issue 2240 explicit Explicitly integrate variables which appear nowhere else on the rhs Oct 6, 2022
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 changes look good, some style comments. Let's see what the tests say. Could maybe do with a couple more tests (e.g. testing the output of an explicit integration - unless that is now done somewhere else by one of the existing tests?)

pybamm/expression_tree/unary_operators.py Outdated Show resolved Hide resolved
pybamm/solvers/solution.py Outdated Show resolved Hide resolved
pybamm/solvers/solution.py Show resolved Hide resolved
pybamm/discretisations/discretisation.py Show resolved Hide resolved
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.

Thanks, looks good (once tests pass)

@valentinsulzer
Copy link
Member

Just had a thought, it might be worth special-casing the case where the rhs is Scalar(0), and in that case simply replacing the variable with its initial condition (instead of the ExplicitTimeIntegral)

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.

A few more comments from looking more closely ...

pybamm/discretisations/discretisation.py Outdated Show resolved Hide resolved
pybamm/discretisations/discretisation.py Outdated Show resolved Hide resolved
pybamm/discretisations/discretisation.py Outdated Show resolved Hide resolved
pybamm/discretisations/discretisation.py Outdated Show resolved Hide resolved
pybamm/discretisations/discretisation.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 10, 2022

Codecov Report

Base: 99.65% // Head: 99.66% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (87c0943) compared to base (ba5187a).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2348      +/-   ##
===========================================
+ Coverage    99.65%   99.66%   +0.01%     
===========================================
  Files          361      269      -92     
  Lines        19948    20190     +242     
===========================================
+ Hits         19879    20123     +244     
+ Misses          69       67       -2     
Impacted Files Coverage Δ
pybamm/discretisations/discretisation.py 99.81% <100.00%> (+0.01%) ⬆️
...ybamm/expression_tree/operations/evaluate_julia.py 100.00% <100.00%> (ø)
pybamm/expression_tree/unary_operators.py 100.00% <100.00%> (ø)
pybamm/input/parameters/lead_acid/Sulzer2019.py 100.00% <100.00%> (ø)
pybamm/input/parameters/lithium_ion/Ai2020.py 100.00% <100.00%> (ø)
pybamm/input/parameters/lithium_ion/Chen2020.py 100.00% <100.00%> (ø)
...input/parameters/lithium_ion/Chen2020_composite.py 100.00% <100.00%> (ø)
pybamm/input/parameters/lithium_ion/Ecker2015.py 100.00% <100.00%> (ø)
pybamm/input/parameters/lithium_ion/Marquis2019.py 100.00% <100.00%> (ø)
pybamm/input/parameters/lithium_ion/Mohtat2020.py 100.00% <100.00%> (ø)
... and 74 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@valentinsulzer valentinsulzer merged commit 7e35372 into pybamm-team:develop Oct 12, 2022
@valentinsulzer
Copy link
Member

@all-contributors add @abillscmu for code

@allcontributors
Copy link
Contributor

@tinosulzer

I've put up a pull request to add @abillscmu! 🎉

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.

Separate differential states that can be explicitly integrated
2 participants