Skip to content

Conversation

pipliggins
Copy link
Contributor

@pipliggins pipliggins commented Jun 25, 2025

Description

This PR addresses an issue encountered when using first_state or last_state solution objects with the output_variables option in the IDAKLU solver - for example, when supplying an starting solution to .solve() within an experiment.

When adding a first_state or last_state solution to a new solution computed with the output_variables option, a conflict arises due to the differing types of processed variables:

  • first_state/last_state solutions dynamically generate ProcessedVariable objects on demand.
  • New solutions (e.g. from .solve()) use ComputedProcessedVariable objects.
    These two classes currently share no functionality and cannot interoperate. Consequently, attempting to combine them to form a new solution results in failure.

This PR introduces a new abstract base class for both ProcessedVariable and ComputedProcessedVariable, providing a shared _update() method. This enables consistent combination of these objects when merging solutions. The concatenated object is always a ComputedProcessedVariable, since converting to a ProcessedVariable is not viable - it requires access to the full state vector, which is unavailable when output_variables is used.

Fixes #4743

Important checks:

Please confirm the following before marking the PR as ready for review:

  • No style issues: nox -s pre-commit
  • All tests pass: nox -s tests
  • The documentation builds: nox -s doctests
  • Code is commented for hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

Copy link

codecov bot commented Jun 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.12%. Comparing base (1ce4ef2) to head (b9c351b).
Report is 1 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #5076   +/-   ##
========================================
  Coverage    99.12%   99.12%           
========================================
  Files          304      305    +1     
  Lines        23576    23608   +32     
========================================
+ Hits         23369    23401   +32     
  Misses         207      207           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@pipliggins pipliggins marked this pull request as ready for review June 26, 2025 14:28
@pipliggins pipliggins requested a review from a team as a code owner June 26, 2025 14:28
@pipliggins pipliggins requested a review from martinjrobins June 26, 2025 14:28
martinjrobins
martinjrobins previously approved these changes Jun 30, 2025
Copy link
Contributor

@martinjrobins martinjrobins left a comment

Choose a reason for hiding this comment

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

looks good, thanks @pipliggins, just a pedantic suggestion on the method name below

@martinjrobins martinjrobins merged commit 1c1dd75 into pybamm-team:develop Jul 3, 2025
21 checks passed
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 SOLVER output_variables long simulation and state issues
2 participants