Repository navigation
fix: identify drive-cycle steps by their contents, not their shortened repr - #6018
Open
brosaplanella wants to merge 1 commit into
Open
brosaplanella wants to merge 1 commit into
brosaplanella wants to merge 1 commit into
Conversation
…d repr numpy shortens the repr of large arrays, so steps with long drive cycles that differ only away from their ends were merged into one. Co-Authored-By: Claude Opus 5.5 <[email protected]>
brosaplanella
marked this pull request as ready for review
November 1, 2026 10:34
Contributor
|
| Project | PyBaMM |
| Branch | fix/experiment-drive-cycle-identity |
| Testbed | bare-metal |
🐰 View full continuous benchmarking report in Bencher
⚠️ WARNING: Truncated view!The full continuous benchmarking report exceeds the maximum length allowed on this platform.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6018 +/- ##
==========================================
- Coverage 98.23% 98.22% -0.01%
==========================================
Files 350 350
Lines 36173 36184 +11
==========================================
+ Hits 35533 35543 +10
- Misses 640 641 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4 tasks done
This branch has not been deployed
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.
numpy shortens the repr of large arrays, so steps with long drive cycles that differ only away from their ends were merged into one.
Description
Steps were identified by text:
Experimentkeyed its processed steps onrepr(step), andBaseStep.__eq__,__hash__andbasic_reprusedhash_args, built fromf"{value}". numpy shortens the repr of arrays over 1000 entries to their first and last three rows, so two drive cycles of over 500 rows that differ only in between gave the same step.Experimentreplaced the second with a copy of the first, and the simulation ran the first drive cycle twice, in both experiment modes, with no error or warning.Changes
hash_argsidentifies an array value by its shape, dtype and SHA-256 digest (_value_identityinbase_step.py), so__eq__,__hash__andbasic_reprtell such steps apart. The model and branch keys inSimulationusebasic_repr, so they follow. Other values keep their text, andrepris unchanged, for display.Experimentkeys the processed steps on(repr(step), step.basic_repr())for step objects, rather than onrepr(step)alone, so steps keep every argument thatreprshows and still tell long drive cycles apart. String steps keep theirreprkey.Tests
TestExperimentSteps.test_drive_cycles_differing_in_the_middle_not_equal: equal drive cycles give equal steps and hashes, and drive cycles that differ in the middle do not.TestSimulationExperiment.test_drive_cycles_differing_in_the_middle[legacy/unified]: the experiment keeps both steps, and the second runs at 5 A at t = 500 s.Fixes #6017
Type of change
Bug fix
Important checks:
Please confirm the following before marking the PR as ready for review:
nox -s pre-commitnox -s testsnox -s doctests