Ensure step and cycle lengths match

[NicolaCourtier]

Description

A one-line fix to ensure that the step length is not longer than the cycle length.

Type of change

Add an entry under # [Unreleased] in CHANGELOG.md, in one of ## Breaking changes, ## Deprecated, ## Features, or ## Bug fixes (include PR number; breaking and deprecation entries need a one-line migration note). Internal-only PRs — refactor, docs, CI, tests — can skip this. See RELEASE.md for the full policy.

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

[NicolaCourtier]

Small fix, ready for review (@rtimms ?)

[rtimms]

Diagnosis is right — make_cycle_solution stores the unsliced step solutions on cycle.steps, while cycle.t comes from from_sub_solutions, which drops the duplicated leading sample. Using exact == correctly mirrors that.

1. Cross-cycle boundaries are still off

"Cycle" is built from np.ones_like(cycle.t) with no dedup, but the parent solution is also from_sub_solutions(cycle_solutions), so it drops the repeat at each cycle boundary too. The j > 0 guard skips exactly that case. Two cycles × two steps, all boundaries exactly repeated:

data len : 197   t len: 197
Cycle len: 198
Step  len: 198
csv FAILED: ValueError All arrays must be of the same length

Step now agrees with Cycle, and both still disagree with the data. save_data(to_format="csv") is the user-visible failure.

Carrying one boundary across the flattened step list fixes both, and lets "Cycle" inherit the dedup instead of needing its own mechanism:

# Save cycle number and step number if the solution has them
if cycles_and_steps and len(self.cycles) > 0:
    cycle_numbers, step_numbers, previous_last_t = [], [], None
    for i, cycle in enumerate(self.cycles):
        for j, step in enumerate(cycle.steps):
            # a step's first point is dropped when it repeats the previous
            # step's last point, so don't number it twice
            repeated = (
                previous_last_t is not None and step.t[0] == previous_last_t
            )
            previous_last_t = step.t[-1]
            length = len(step.t) - repeated
            cycle_numbers.append(np.full(length, i, dtype=float))
            step_numbers.append(np.full(length, j, dtype=float))
    data_short_names["Cycle"] = np.concatenate(cycle_numbers)
    data_short_names["Step"] = np.concatenate(step_numbers)

Also one np.concatenate at the end rather than one per step, which matters on long cycling runs.

2. The test asserts the wrong invariant

np.zeros(50), np.ones(49) hardcodes the answer. What matters is len(Step) == len(Cycle) == len(data["c"]) — assert that, add a two-cycle case, and the gap above gets caught. As written the test passes on a solution that still can't be written to csv.

3. Nit

np.ones_like(step.t[first]) slices an array purely for its shape.

---

Worth noting for severity: normal experiments don't hit this. BaseSolver.step nudges each step start with np.nextafter, so real Simulation.solve runs came out consistent in every configuration I tried (event-terminated, fixed period, multi-cycle). It bites when solutions are assembled directly — sol1 + sol2, hand-built cycles, starting_solution — which is exactly what the test does.

[rtimms]

Thanks @NicolaCourtier ! Just a couple of comments

[NicolaCourtier]

Thanks Rob, cc. @SarahRo

[rtimms]

Thanks @NicolaCourtier @SarahRo, this now handles the boundaries between cycles too. The length check in the test catches the old bug: it fails on main, and save_data(to_format="csv") works on this branch. Approving, with three small things to do before merge:

1. Please add a ## Bug fixes entry under # [Unreleased] in CHANGELOG.md, e.g.
   - `Solution.get_data_dict` and `save_data` now give the "Cycle" and "Step" columns the same length as the data when a step or cycle repeats the previous one's last time point. ([#5724](https://github.com/pybamm-team/PyBaMM/pull/5724))
2. tmp_path isn't used in test_get_data_cycles_steps, so please drop it. To cover the failure users actually see, add sol.save_data(variables=["c"], to_format="csv"). Without a filename it returns the CSV string, so no temp file is needed.
3. Please swap pybamm.ScipySolver() for pybamm.IDAKLUSolver() in that test, since ScipySolver is deprecated.

[github-actions]

Bencher Report

Project	PyBaMM
Branch	step-cycle-numbers
Testbed	bare-metal

⚠️ WARNING: No benchmarks found!

🐰 View full continuous benchmarking report in Bencher

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.12%. Comparing base (9878b13) to head (98455d5).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5724   +/-   ##
=======================================
  Coverage   98.12%   98.12%           
=======================================
  Files         348      348           
  Lines       35575    35579    +4     
=======================================
+ Hits        34909    34913    +4     
  Misses        666      666           

☔ View full report in Codecov by Harness.
📢 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.

[SarahRo]

Thanks @NicolaCourtier @SarahRo, this now handles the boundaries between cycles too. The length check in the test catches the old bug: it fails on main, and save_data(to_format="csv") works on this branch. Approving, with three small things to do before merge:

1. Please add a `## Bug fixes` entry under `# [Unreleased]` in `CHANGELOG.md`, e.g.
   `` - `Solution.get_data_dict` and `save_data` now give the "Cycle" and "Step" columns the same length as the data when a step or cycle repeats the previous one's last time point. ([#5724](https://github.com/pybamm-team/PyBaMM/pull/5724)) ``

2. `tmp_path` isn't used in `test_get_data_cycles_steps`, so please drop it. To cover the failure users actually see, add `sol.save_data(variables=["c"], to_format="csv")`. Without a filename it returns the CSV string, so no temp file is needed.

3. Please swap `pybamm.ScipySolver()` for `pybamm.IDAKLUSolver()` in that test, since `ScipySolver` is deprecated.

@NicolaCourtier @rtimms Thanks! I've updated the test and changelog.