Fix Solution.get_data_dict() Step length mismatch with starting_solution (#4990)#5479
Open
kamal20122012 wants to merge 1 commit intopybamm-team:mainfrom
Open
Conversation
…nsistent When a Simulation is resumed via starting_solution and the first experiment step's event triggers at initial conditions, the simulation loop appends a pybamm.EmptySolution placeholder (with a single time equal to start_time) to the cycle's step list. Solution.__add__ deduplicates that boundary point against the carried-over last_state, so the placeholder timestamp is absent from cycle.t. get_data_dict, however, walked cycle.steps and concatenated np.ones_like(step.t) for every step including the EmptySolution, inflating the "Step" array by one element per such cycle relative to "Cycle" and "Time [s]". Skip EmptySolution entries when building the "Step" array so its length matches cycle.t. Adds a regression test that mimics the placeholder scenario and asserts equal lengths across "Step", "Cycle", and the data variable. Fixes pybamm-team#4990
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5479 +/- ##
=======================================
Coverage 98.15% 98.15%
=======================================
Files 338 338
Lines 31115 31124 +9
=======================================
+ Hits 30542 30551 +9
Misses 573 573 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Simulationis resumed viastarting_solutionand the very first experiment step's event triggers at the cycle's initial conditions, the simulation loop appends apybamm.EmptySolutionplaceholder (with a single time equal tostart_time) to that cycle's step list.Solution.__add__deduplicates that boundary timestamp against the carried-overlast_state, so the placeholder time is not present incycle.t.Solution.get_data_dictpreviously concatenatednp.ones_like(step.t)for every entry incycle.stepsincluding theEmptySolution, which inflateddata["Step"]by exactly one element per such cycle relative todata["Cycle"]anddata["Time [s]"].EmptySolutionentries when building"Step"so its length stays consistent withcycle.t.Fixes #4990
Test plan
Step,Cycle, andTime [s]are all 1763 (previously 1764 / 1763 / 1763).tests/unit/test_solvers/test_solution.py::TestSolution::test_get_data_cycles_steps_skips_empty_step— verified to fail on pre-fix code (Step=7vsCycle=6) and pass with the fix.test_get_data_cycles_stepsstill passes.pytest tests/unit/test_solvers/test_solution.py— 55 passed (1 pre-existing skipped:test_plotrequires optionalmatplotlib).pre-commit run --files <changed files>— all hooks pass.