[galenlynch]

get_chunk_with_margin can return a view of parent data when no padding or casting is needed. Moving tapering outside that helper therefore requires an explicit way to obtain a buffer that callers can safely modify.

This extracts a private _taper_margins(traces, window) helper and adds copy=True to both margin helpers. phase_shift requests an owned buffer and builds its raised-cosine window once per segment, reusing it across chunks. Existing window_on_margin=True behavior remains supported.

Split out from #4563 because the FIR implementation no longer depends on this refactor.

Validation: six focused tests pass, including exact agreement with the legacy FFT path for int16, float32, and float64 inputs, and checks that read-only parent arrays remain untouched at interior and boundary chunks.

[JoeZiminski]

Thanks @galenlynch this is very nice, will review next week (and the cool FIR phase shift implementation) but wanted to ask here because I was thinking about it recently - do you think how the get_data()/get_traces() handle returning a copy or view could be generally made more explicit in SpikeInterface? I was surprised to learn recently that get_traces() can return a view at all, but also a copy under some conditions.

e.g. this can happen:

cached = recording.save(format="memory", sharedmem=False)
traces = cached.get_traces(start_frame=0, end_frame=100)
traces[:] = 0  # Also zeros those samples in cached

(this mostly effects NumpyRecordings, fortunately binary data are read-only, but nonetheless this could be brittle if people test new features on simulated data and write operations in-place, then crashes on binary data when released).

It would be nice to enforce a copy at the root get_traces() body and then all consuming functions can safely assume the received traces is a copy. Otherwise, we may (and probably are) repeatedly copying data chunks through the preprocessing chain to protect against the possibility of receiving a view. WDTY? If you agree maybe we can promote the copy to the core get_traces() (this could happen later, after this PR is merged).

[alejoe91]

Thanks @galenlynch this is very nice, will review next week (and the cool FIR phase shift implementation) but wanted to ask here because I was thinking about it recently - do you think how the get_data()/get_traces() handle returning a copy or view could be generally made more explicit in SpikeInterface? I was surprised to learn recently that get_traces() can return a view at all, but also a copy under some conditions.

e.g. this can happen:

cached = recording.save(format="memory", sharedmem=False)
traces = cached.get_traces(start_frame=0, end_frame=100)
traces[:] = 0  # Also zeros those samples in cached

(this mostly effects NumpyRecordings, fortunately binary data are read-only, but nonetheless this could be brittle if people test new features on simulated data and write operations in-place, then crashes on binary data when released).

It would be nice to enforce a copy at the root get_traces() body and then all consuming functions can safely assume the received traces is a copy. Otherwise, we may (and probably are) repeatedly copying data chunks through the preprocessing chain to protect against the possibility of receiving a view. WDTY? If you agree maybe we can promote the copy to the core get_traces() (this could happen later, after this PR is merged).

I agree! We should make the NumpyRecordingSegment.get_traces()always set thewritableflag toFalse`. @galenlynch can you add it to this PR?

This will also fix #4563

[galenlynch]

Thanks @JoeZiminski! Agreed: the mix of views and copies is a trap, as your NumpyRecording example shows. Instead of always copying, how about using numpy's writeable flag to say who owns the array: a caller may modify any writable array it gets back, and copies a read-only one first.

• Sources that share memory return read-only views, as the binary memmap and SharedMemoryRecording already do. Once NumpyRecording does too, your example raises on simulated data just as it does on binary data.
• Code that writes in place copies only if traces.flags.writeable is False. clip, interpolate_bad_channels, remove_artifacts, silence_periods, highpass_spatial_filter and copy=True here all copy every time today, so each would skip a copy whenever its parent returns a fresh array.
• One test keeps sources honest: call get_traces twice on the same frames and assert that the result is read-only or that the two calls don't share memory. Running that over the extractors the suite already builds would catch a writable view without knowing the source.

I'd avoid always copying because most nodes already return a fresh array, so a copy in every get_traces() would add an expensive pass over memory to every read. Preprocessors also call their parent segment directly, so the copy would have to go in every segment's get_traces, not in BaseRecording.get_traces.

Code that writes into NumpyRecording traces on purpose would start raising, so this needs a release note.

Happy to do it in a follow-up. copy=True here is correct either way.