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

Fix IDAKLU output_variables to work with extrapolation events #4440

Merged

Conversation

pipliggins
Copy link
Contributor

Fixes #4439

Similarly to #3937 this is caused by code searching for variables in the full state vector which aren't present due to the reduced set produced when output_variables are specified.

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 Sep 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.46%. Comparing base (7be637c) to head (f5717ff).
Report is 9 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #4440   +/-   ##
========================================
  Coverage    99.46%   99.46%           
========================================
  Files          293      293           
  Lines        22381    22384    +3     
========================================
+ Hits         22261    22264    +3     
  Misses         120      120           

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

@MarkVeerasingam
Copy link

@pipliggins - just tried your edit, everything works well on my end, thanks for your help on this! :)

@MarcBerliner MarcBerliner merged commit d362c98 into pybamm-team:develop Sep 20, 2024
26 checks passed
MarcBerliner added a commit to MarcBerliner/PyBaMM that referenced this pull request Sep 22, 2024
commit d362c98
Merge: 7be637c f5717ff
Author: Marc Berliner <[email protected]>
Date:   Fri Sep 20 10:27:14 2024 -0400

    Merge pull request pybamm-team#4440 from pipliggins/output_vars_extrapolation

    Fix IDAKLU output_variables to work with extrapolation events

commit f5717ff
Merge: b012684 7be637c
Author: Marc Berliner <[email protected]>
Date:   Fri Sep 20 09:21:26 2024 -0400

    Merge branch 'develop' into output_vars_extrapolation

commit b012684
Author: Pip Liggins <[email protected]>
Date:   Thu Sep 19 17:16:35 2024 -0700

    Switch test to triggered event

commit 7be637c
Author: Marc Berliner <[email protected]>
Date:   Thu Sep 19 15:39:27 2024 -0400

    Faster (re)initialization of ODEs in `IDA` (pybamm-team#4453)

    * Fast ODE reinitialization

    * Update CHANGELOG.md

    * remove redundant `IDAReInit` call

    * address comments

    * Update IDAKLUSolverOpenMP.inl

commit cee2cad
Author: Pip Liggins <[email protected]>
Date:   Thu Sep 19 10:22:21 2024 -0700

    update changelog

commit 37c94f9
Author: Pip Liggins <[email protected]>
Date:   Thu Sep 19 10:10:32 2024 -0700

    Add test

commit 06d7ecc
Merge: f47c12e 4cda488
Author: Pip Liggins <[email protected]>
Date:   Thu Sep 19 09:25:57 2024 -0700

    Merge branch 'develop' into output_vars_extrapolation

commit 4cda488
Merge: 48dbb68 5bb146f
Author: Valentin Sulzer <[email protected]>
Date:   Thu Sep 19 02:15:59 2024 -0700

    Merge pull request pybamm-team#4330 from parkec3/ocvr_ecm

    ECM with split OCV

commit 5bb146f
Merge: 712a3ee 48dbb68
Author: Valentin Sulzer <[email protected]>
Date:   Thu Sep 19 00:19:24 2024 -0700

    Merge branch 'develop' into ocvr_ecm

commit 48dbb68
Author: Martin Robinson <[email protected]>
Date:   Wed Sep 18 22:21:38 2024 +0100

    feat: add OpenMP parallelization to IDAKLU solver for lists of input parameters (pybamm-team#4449)

    * new solver option `num_solvers`, indicates how many solves run in parallel
    * existing `num_threads` gives total number of threads which are distributed among `num_solvers`

commit e1118ec
Author: Marc Berliner <[email protected]>
Date:   Wed Sep 18 12:35:47 2024 -0400

    Update CODEOWNERS (pybamm-team#4452)

commit f47c12e
Merge: d6b213c 05a0b24
Author: Pip Liggins <[email protected]>
Date:   Mon Sep 16 17:36:13 2024 -0700

    Merge branch 'develop' into output_vars_extrapolation

commit 05a0b24
Author: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Date:   Mon Sep 16 22:08:33 2024 +0100

    chore: update pre-commit hooks (pybamm-team#4445)

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

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

commit 8e3eb31
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Mon Sep 16 16:00:04 2024 -0400

    Build(deps): bump github/codeql-action in the actions group (pybamm-team#4444)

    Bumps the actions group with 1 update: [github/codeql-action](https://github.com/github/codeql-action).

    Updates `github/codeql-action` from 3.26.6 to 3.26.7
    - [Release notes](https://github.com/github/codeql-action/releases)
    - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md)
    - [Commits](github/codeql-action@4dd1613...8214744)

    ---
    updated-dependencies:
    - dependency-name: github/codeql-action
      dependency-type: direct:production
      update-type: version-update:semver-patch
      dependency-group: actions
    ...

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

commit d6b213c
Author: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Date:   Fri Sep 13 23:14:56 2024 +0000

    style: pre-commit fixes

commit 343cea4
Author: Pip Liggins <[email protected]>
Date:   Fri Sep 13 15:51:17 2024 -0700

    change check_extrapolation to use t/y_event

commit 712a3ee
Merge: 3b21bf6 6c1815b
Author: Valentin Sulzer <[email protected]>
Date:   Sat Sep 7 13:10:30 2024 -0700

    Merge branch 'develop' into ocvr_ecm

commit 3b21bf6
Author: Eric G. Kratz <[email protected]>
Date:   Tue Sep 3 16:45:19 2024 -0400

    Move changelog update to unreleased

commit 4786443
Merge: 13270c2 ac93806
Author: Eric G. Kratz <[email protected]>
Date:   Tue Sep 3 16:44:14 2024 -0400

    Merge branch 'develop' into ocvr_ecm

commit 13270c2
Merge: f23ac77 1ab27d1
Author: Eric G. Kratz <[email protected]>
Date:   Tue Sep 3 13:20:33 2024 -0400

    Merge branch 'develop' into ocvr_ecm

commit f23ac77
Author: Caitlin Parke <[email protected]>
Date:   Tue Sep 3 13:11:22 2024 -0400

    updated tests and naming convention

commit b820e59
Merge: 9a996c4 ac6c450
Author: Eric G. Kratz <[email protected]>
Date:   Wed Aug 28 16:20:47 2024 -0400

    Merge branch 'develop' into ocvr_ecm

commit 9a996c4
Author: Caitlin Parke <[email protected]>
Date:   Wed Aug 21 16:46:28 2024 -0400

    updated for pytest and better coverage

commit e4504a6
Author: Caitlin Parke <[email protected]>
Date:   Tue Aug 20 18:18:42 2024 -0400

    added docs

commit b4c1897
Merge: 3f1e7f5 977dcf9
Author: Caitlin D. Parke <[email protected]>
Date:   Mon Aug 19 20:09:15 2024 -0400

    Merge branch 'pybamm-team:develop' into ocvr_ecm

commit 3f1e7f5
Author: Caitlin Parke <[email protected]>
Date:   Mon Aug 19 20:07:44 2024 -0400

    added default plotting variables

commit 84fba69
Merge: b1de9d1 99e3119
Author: Caitlin Parke <[email protected]>
Date:   Fri Aug 16 00:44:34 2024 -0400

    Merge branch 'ocvr_ecm' of https://github.com/parkec3/PyBaMM into ocvr_ecm

commit b1de9d1
Author: Caitlin Parke <[email protected]>
Date:   Fri Aug 16 00:44:19 2024 -0400

    pre-commit

commit 99e3119
Author: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Date:   Fri Aug 16 04:42:19 2024 +0000

    style: pre-commit fixes

commit 758175f
Author: Caitlin Parke <[email protected]>
Date:   Fri Aug 16 00:42:04 2024 -0400

    added tests

commit 1fc5670
Merge: 3daabce 1e3f139
Author: Caitlin D. Parke <[email protected]>
Date:   Fri Aug 16 00:40:38 2024 -0400

    Merge branch 'pybamm-team:develop' into ocvr_ecm

commit 3daabce
Author: Caitlin Parke <[email protected]>
Date:   Wed Aug 14 12:01:58 2024 -0400

    parameter updates

commit a502d17
Merge: dbeaeb5 9691d09
Author: Caitlin D. Parke <[email protected]>
Date:   Wed Aug 14 11:58:58 2024 -0400

    Merge branch 'pybamm-team:develop' into ocvr_ecm

commit dbeaeb5
Author: Caitlin Parke <[email protected]>
Date:   Tue Aug 13 14:45:42 2024 -0400

    still working

commit 3bc77c2
Merge: 2a945f2 83239a3
Author: Caitlin Parke <[email protected]>
Date:   Mon Aug 12 11:25:01 2024 -0400

    Merged remote into local changes

commit 2a945f2
Author: Caitlin Parke <[email protected]>
Date:   Mon Aug 12 11:17:11 2024 -0400

    Added parameter objects in model

commit 83239a3
Author: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Date:   Fri Aug 9 20:43:34 2024 +0000

    style: pre-commit fixes

commit f93e67b
Author: Caitlin Parke <[email protected]>
Date:   Fri Aug 9 16:39:33 2024 -0400

    pre-commit changes

commit 80428c9
Merge: f3faca4 b1fc595
Author: Caitlin D. Parke <[email protected]>
Date:   Fri Aug 9 16:34:30 2024 -0400

    Merge branch 'pybamm-team:develop' into ocvr_ecm

commit f3faca4
Author: Caitlin Parke <[email protected]>
Date:   Fri Aug 9 16:33:10 2024 -0400

    Added ECM + split OCV model
MarkVeerasingam added a commit to MarkVeerasingam/Battery-Simulator that referenced this pull request Oct 16, 2024
  "error": "An unexpected error occurred: y is too short, so value with slice is smaller than expected"
}

Issue: Pybamm handled this in a pr:  pybamm-team/PyBaMM#4440

pr was added but new version of pybamm not released yet
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.

[Bug]: IDAKLU output_variables fails when checking for interpolant_extrapolation events
3 participants