[briangow]

As mentioned in #557, we should add segment validation when writing multi-segment records. This PR adds pre-write validation to MultiRecord.wrsamp() by activating the previously unused _check_segment_cohesion() method, which is now called before any files are written.

The validation includes checks for basic segment cohesion (matching n_seg, sampling frequencies, and segment lengths) as well as new validation for expanded signals. For segments with e_d_signal or e_p_signal, the method now verifies that samps_per_frame is properly set for all channels and that expanded signal lengths match expected values (samps_per_frame[ch] * sig_len). This provides clear, descriptive error messages when segments are misconfigured, preventing invalid multi-segment records from being written.

[tompollard]

Thanks Brian, checking all the segments before writing anything is a good change. On main, a segment whose length doesn't match seg_len gets written without any error, and a bad segment partway through leaves half-written files. This PR fixes both.

I tried a few cases locally, and two writes that work on main are now rejected:

1. The expanded check runs even when expanded=False. A segment with a valid d_signal and an out-of-date e_d_signal now fails, even though only d_signal is written. The check also falls back to e_p_signal, which is never written. Could we pass expanded into _check_segment_cohesion and only check e_d_signal when it's True?
2. samps_per_frame=[None] is rejected. Record.wrsamp(expanded=True) treats a missing value as 1 (check_sig_cohesion in _signal.py), so a segment that writes fine on its own now fails inside a multi-segment record. It would be good to treat it the same way here.

Both of these come from having the expanded-length rule in two places. Another option is to drop the new block and run each segment's existing checks (its header field checks plus check_sig_cohesion) in a first pass, then write everything in a second pass. That keeps the "check before writing" behaviour with a single rule.

A few smaller things:

• Gap segments (None, e.g. from reading a variable-layout record with sampto) now fail with 'NoneType' object has no attribute 'fs'. That isn't worse than before, but a clear error saying gap segments can't be written yet would help.
• There are no tests for MultiRecord.wrsamp yet. It would be good to add:
  • a round trip of 041s
  • a seg_len mismatch
  • a bad segment after a good one, checking that no files are written
  • the two cases above that should be accepted
• Could you add a changelog entry to docs/?
• A few of the new error message lines are over 88 characters.

[briangow]

Thanks for suggesting a first pass that runs each segment’s existing header and signal checks before writing. I didn’t take that approach in this PR because those checks are currently part of Record.wrsamp()’s write flow. Preflighting every segment would require changing or duplicating that flow, which is broader and riskier than the targeted multi-record cohesion validation here. We can consider a larger refactor of Record.wrsamp() at another time.

I’ve updated the PR to:

• Keep _check_segment_cohesion() focused on properties relating each segment to the multi-record header, and remove the separate expanded-signal length rule so the existing record-level signal checks remain the source of truth.
• Skip empty (~) segments in _check_segment_cohesion(), since main now supports writing records with empty segments and they have no Record to check.
• Add tests for an 041s round trip, segment-length mismatch, and the two previously rejected-but-valid cases.
• Add a changelog entry.

Other validators may check additional properties or parameters later in the write process, so this doesn’t catch every possible validation error before files are written. If we find a specific case where a later validation causes partial output, we can add the relevant pre-write check to _check_segment_cohesion() where it makes sense.

[tompollard]

Thanks, this looks good! Two small things:

• I don't see the changelog fragment. Please could you add it?
• Because _check_segment_cohesion() now runs before the header field checks, a seg_len that is shorter than n_seg gives IndexError: list index out of range. On main it gives ValueError: The length of field: seg_len does not match field n_seg. Perhaps we could add a length check next to the n_seg one?:

if len(self.seg_len) != self.n_seg:
    raise ValueError("Length of seg_len must match the 'n_seg' field")

[briangow]

Good catch @tompollard . This has been updated and a test has been added.

[tompollard]

Thanks, this looks great! Happy to merge.