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

delay xarray.DataArray initialization #3862

Merged
merged 4 commits into from
Mar 5, 2024

Conversation

valentinsulzer
Copy link
Member

Description

This speeds up simulations by 5x because xarray.DataArray initialization is slow
Before (560ms, 2% time spent solving the model):
Screenshot 2024-03-04 at 4 42 08 PM
After (130ms, 10% time spent solving the model):
Screenshot 2024-03-04 at 4 42 24 PM

Test code (this example is a "worst-case scenario" for overheads where solving the simulation is very quick, it highlights the other parts of the code that are slow):

import pybamm
import matplotlib.pyplot as plt

pybamm.set_logging_level("NOTICE")
experiment = pybamm.Experiment(
    [
        (
            "Discharge at 1C for 1 minute",
            "Rest for 1 minute",
            # "Charge at 5 A until 4.2 V",
            # "Hold at 4.2 V until 10 mA",
            # "Rest for 1 hour",
        ),
    ]
    * 10
)
model = pybamm.lithium_ion.SPM()
parameter_values = pybamm.ParameterValues("Chen2020")

sim = pybamm.Simulation(
    model,
    experiment=experiment,
    parameter_values=parameter_values,
    solver=pybamm.CasadiSolver("fast with events"),
)
sim.solve()

There are still some significant overheads, we should be as close to 100% time spent solving the model as possible, but this is a step in the right direction.

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: $ pre-commit run (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
  • All tests pass: $ python run-tests.py --all (or $ nox -s tests)
  • The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s quick).

Further checks:

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

Copy link

codecov bot commented Mar 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.60%. Comparing base (ed6df7e) to head (0767c8d).
Report is 1 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3862      +/-   ##
===========================================
- Coverage    99.60%   99.60%   -0.01%     
===========================================
  Files          259      259              
  Lines        21284    21272      -12     
===========================================
- Hits         21200    21188      -12     
  Misses          84       84              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@kratman kratman left a comment

Choose a reason for hiding this comment

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

I don't see any issues with this and the tests pass

@valentinsulzer valentinsulzer merged commit 7a82687 into develop Mar 5, 2024
46 checks passed
@valentinsulzer valentinsulzer deleted the delay-xarray-initialization branch March 5, 2024 15:45
valentinsulzer added a commit that referenced this pull request Mar 6, 2024
agriyakhetarpal pushed a commit that referenced this pull request Mar 6, 2024
* update changelog for #3862

* reword
lorenzofavaro pushed a commit to lorenzofavaro/PyBaMM that referenced this pull request Mar 13, 2024
agriyakhetarpal added a commit that referenced this pull request Mar 13, 2024
* Enable flake8-bugbear

Fix unused-loop-control-variable (B007)

Fix function-uses-loop-variable (B023)

Fix raise-without-from-inside-except (B904)

Fix mutable-argument-default (B006)

Fix no-explicit-stacklevel (B028)

Fix get-attr-with-constant (B009)

Fix loop-variable-overrides-iterator (B020)

Fix unary-prefix-increment-decrement (B002)

Fix function-call-in-default-argument (B008)

Fix cached-instance-method (B019)

* Show exception chain (B904)

* Disable single lint check (B019)

* delay xarray.DataArray initialization

* revert example

* Bump the actions group with 1 update (#3861)

Bumps the actions group with 1 update: [awalsh128/cache-apt-pkgs-action](https://github.com/awalsh128/cache-apt-pkgs-action).


Updates `awalsh128/cache-apt-pkgs-action` from 1.4.1 to 1.4.2
- [Release notes](https://github.com/awalsh128/cache-apt-pkgs-action/releases)
- [Commits](awalsh128/cache-apt-pkgs-action@v1.4.1...v1.4.2)

---
updated-dependencies:
- dependency-name: awalsh128/cache-apt-pkgs-action
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: actions
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Make `typing-extensions` and `sympy` required dependencies to fix PyBaMM import (#3848)

* import `typing_extensions` as optional_dependency

* Make `typing-extensions` a required dependency

* Try forward referencing for `sympy` in `IndependentVariable`

* Make `sympy` a required dependency

* Update docs for SymPy

* Import `sympy` without `have_optional_dependency`

* Import `sympy` without `TYPE_CHECKING`

* Changelog

* Update CHANGELOG.md

Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>

* Rearrange position in changelog

---------

Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
Co-authored-by: Eric G. Kratz <kratman@users.noreply.github.com>

* Rename have_optional_dependency (#3866)

* Rename have_optional_dependency

* Change log

* Fix import

* style: pre-commit fixes

* Update pybamm/util.py

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Arjun Verma <arjunverma.oc@gmail.com>

* update changelog for #3862 (#3869)

* update changelog for #3862

* reword

* fix deprecation warning and check for electrode/particle diffusivity

* update test

* Edit naming for unused loop control variable (B007)

* #3883 updates to install from source (#3884)

* Disable benchmarks on PRs (#3876)

* Remove`[ latexify` extra (#3888)

* Improve Getting Started notebooks (#3750)

* improve tutorials 1 to 3

* use new syntax for plot_voltage_components

* style: pre-commit fixes

* simplify notebook by removing drive cycle

* add additional comment on dynamic_plot

* improve tutorial 5

* remove the use of solution in notebook 5, this is introduced in notebook 6

* fix minor typo

* minor text updates

* style: pre-commit fixes

* minor text updates plus extra links

* minor text updates!

* minor text updates

* remove tutorial 10 as very similar to tutorial 3 in creating models

* move and rename tutorial 11 to creating models section

* style: pre-commit fixes

* fix minor typo in current_with_time

* implement Rob & Eric's comments

* update docs links

* Valentin's comments

* style: pre-commit fixes

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Eric G. Kratz <kratman@users.noreply.github.com>

* chore: update pre-commit hooks (#3890)

* chore: update pre-commit hooks

updates:
- [github.com/astral-sh/ruff-pre-commit: v0.2.2 → v0.3.2](astral-sh/ruff-pre-commit@v0.2.2...v0.3.2)

* style: pre-commit fixes

* Update .git-blame-ignore-revs

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>

* style: pre-commit fixes

* Resolve conflicts (B904, B028, B020)

* Added a schedule on `needs-reply remove` workflow  (#3891)

* added schedule to need-reply workflow

* added schedule to need-reply workflow

---------

Co-authored-by: Arjun Verma <arjunverma.oc@gmail.com>

* style: pre-commit fixes

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Valentin Sulzer <valentinsulzer@hotmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Arjun Verma <arjunverma.oc@gmail.com>
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
Co-authored-by: Eric G. Kratz <kratman@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Martin Robinson <martinjrobins@gmail.com>
Co-authored-by: Ferran Brosa Planella <Ferran.Brosa-Planella@warwick.ac.uk>
Co-authored-by: Santhosh <52504160+santacodes@users.noreply.github.com>
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.

None yet

3 participants