Repository navigation
Discussion: should sampling_rate / sampling_period be constrained to be strictly positive? #1934
Description
Summary
Neo currently accepts negative and zero sampling_rate / sampling_period without complaint, on AnalogSignal, ImageSequence and SpikeTrain. I'd like to discuss whether we should constrain these attributes to be strictly positive, and if so, how to stage the change.
Current behaviour
Neither setter validates the sign (neo/core/analogsignal.py, sampling_rate / sampling_period properties). They check only for None and for the presence of units. The same holds at construction, via _get_sampling_rate.
sig = AnalogSignal(np.arange(10).reshape(10, 1), units="uV",
sampling_rate=-4 * pq.Hz, t_start=4.25 * pq.s)
sig.t_start, sig.t_stop # 4.25 s, 1.75 s -> t_stop < t_start
sig.duration # -2.5
sig.times # [4.25, 4.0, 3.75, ... 2.0] decreasingA zero rate is accepted too, giving sampling_period = inf and t_stop = inf, with only a numpy RuntimeWarning: divide by zero to hint at it.
Note: The constructor for SpikeTrainProxy does check for positive sampling rates for waveforms.
Consequences I've confirmed on master (442658a):
assert_neo_object_is_compliantpasses both the negative and the zero case, so the test suite cannot catch them.time_slicefails silently rather than raising: on the signal above,sig.time_slice(2.5*pq.s, 4*pq.s)returns an empty signal, though 7 samples lie in that window.durationis negative.rescaleandtime_indexpropagate the state without comment.
Where negative rates actually come from
Mostly from reverse slicing. AnalogSignal.__getitem__ multiplies the sampling period by the slice step, so a negative step yields a negative rate. For the tuple form this is long-standing on master:
sig[::-1, 0].sampling_rate # -4.0 Hz (current master)PR #1919, which fixes genuine t_start bugs in the same method, extends that to the bare-slice form (sig[::-1]) and adds a test asserting the negative period. That's a reasonable consistency argument on its own terms, which is what prompted me to open this rather than treat it as a review comment.
Questions to discuss
- Should the constraint be strict positivity?
rate > 0would reject both the negative and the zero/infcase. Is there a legitimate use for either that I'm missing? - What should reverse slicing do instead? Options: raise
NotImplementedErrorfor negative steps (consistent with the existing treatment of array indices a couple of lines above), raiseValueError, or something else? - Backward compatibility.
sig[::-1, 0]has produced a negative rate for a long time, so anyone relying on it would start seeing an exception. Is that acceptable in a minor release, or does it need a deprecation cycle? It would also be worth checking whether any IO reader can pass a negative or zero rate through from a malformed file, since those would start raising exceptions when reading.
Related
As far as I can see, nothing in IrregularlySampledSignal checks for monotonically increasing sample times, but t_start, t_stop and duration are defined positionally, not via min and max. time_slice also assumes sorted sample times.
Activity
zm711 commentedon Oct 27, 2026
1
I can't think of reason why
sampling_ratecould ever be negative. I could imagine a file returning 0 perhaps for a stream that was not recorded (ie you have 10 channels, but used only 9 so one of the channels shows a sampling of 0). But negative seems like bad data that we shouldn't allow to pass.2
I'm neutral since I don't do any reverse slicing at this time. My gut would say it should be allowable because I use reverse slicing for other projects, so it seems like a pretty basic feature, but since I don't use it in a neo context no strong feelings.
3
As far as at the rawio level we only require it to be a float64 (here) so negative is possible. The only check we have is that all channels in a stream have to have the same sampling rate, but no requirement for it to be positive. (see here for function to get sampling rate)
I will say that we will hopefully moved to NumPy 2.0+ before Dec so now is a good time for a major release instead of a point release right? I don't know if @h-mayorquin will have time to fully implement gap tolerance before then and @samuelgarcia has
digital_sigchunkway on the back burner now. So the only reason for major would be the compatibility changes. Could go either way I suppose?Heberto and I have discussed this in the past so he should weigh in too for sure. But I have the feeling that unless someone says they need negative sampling that this a wrong behavior that we should just fix. If people's code errors then that is a sign that they were relying on a bad behavior from our functions which shouldn't be allowed. I would lean toward not deprecating--instead I would prefer to have a warning before the error saying that previously this silently passed but that we now have a hard check. Then we can delete the warning message in a version or two and just have the plain error. something like
tldr
I would allow 0 and positive, but not negative. I think minor is honestly fine, though major is probably safer and can be included in the context of our NumPy support changes.
h-mayorquin commentedon Oct 28, 2026
Concerning 1, I think that
sampling_rateshould be strictly positive. That is, zero should throw an error as well. A zerosampling_rategives an infinitesampling_periodandt_stopso the object is broken from the moment it is created.