Skip to content

Raise the intended error for a zero sampling_period in instantaneous_rate - #710

Open
adityasingh2400 wants to merge 1 commit into
NeuralEnsemble:masterfrom
adityasingh2400:fix/instantaneous-rate-sampling-period-zero
Open

Raise the intended error for a zero sampling_period in instantaneous_rate#710
adityasingh2400 wants to merge 1 commit into
NeuralEnsemble:masterfrom
adityasingh2400:fix/instantaneous-rate-sampling-period-zero

Conversation

@adityasingh2400

Copy link
Copy Markdown

instantaneous_rate accepts a sampling_period of exactly zero and then dies deep inside the bin count computation with an error that says nothing about the argument that caused it.

import neo
import quantities as pq
from elephant.kernels import GaussianKernel
from elephant.statistics import instantaneous_rate

st = neo.SpikeTrain([0.1, 0.3, 0.5, 0.7] * pq.s, t_start=0 * pq.s, t_stop=1 * pq.s)
instantaneous_rate(st, sampling_period=0 * pq.ms, kernel=GaussianKernel(50 * pq.ms))

On current master:

elephant/statistics.py:1032: RuntimeWarning: divide by zero encountered in divide
  n_bins = int(((t_stop - t_start) / sampling_period).simplified)
OverflowError: cannot convert float infinity to integer

The documented contract is already correct, only the validation is wrong. The Raises section of instantaneous_rate says:

    TypeError
        ...
        *  If `sampling_period` is not larger than zero.

"not larger than zero" includes zero, but the guard only rejects negative values, so zero slips through, the duration divided by zero is infinity, and int() raises OverflowError. This widens the guard to <= 0 and reports the intended message, so the same script now ends with ValueError: The 'sampling_period' (0.0 ms) must be larger than zero.

The Raises section is also made self consistent. It listed the zero case under TypeError and a separate "smaller than zero" case under ValueError, while the function has always raised ValueError here. The single bullet now sits under ValueError and reads "not larger than zero", which is both the documented contract and what the code does.

test_instantaneous_rate_errors gains two cases, a single spike train and a list of spike trains, both with sampling_period=0, asserting the ValueError message. Reverting statistics.py to master makes it fail with the OverflowError above. After, 101 passed, 1 skipped and 33 subtests passed. pycodestyle reports no new findings on either changed file.

Unrelated but noticed in the same module while reading: cv2, lv and lvr all document with_nan as Default: True while their signatures use with_nan=False. Happy to send that as a separate docs only change if it is wanted.

Disclosure: this change was prepared with AI assistance. I have reviewed and tested it.

A sampling_period of exactly zero passed the 'sampling_period.magnitude
< 0' guard and reached the bin-count computation, where dividing the
recording duration by zero produced infinity and int() raised
OverflowError: cannot convert float infinity to integer.

The docstring already documents the intended contract, it lists a
sampling_period that is 'not larger than zero' under Raises, so only the
guard was wrong. Widen it to '<= 0' and raise the informative ValueError
instead. The Raises section now lists that case under ValueError, which
is the class the function actually raises for a negative sampling
period.
@adityasingh2400

Copy link
Copy Markdown
Author

The docs job here is the same remote-fetch problem as on my other three PRs in this repo, not something from this branch.

The sphinx build dies executing a notebook that downloads sample data, this time with URLError: [Errno 110] Connection timed out. This branch only touches elephant/statistics.py and its test module.

Counting across the four PRs, that fetch has now failed five different ways in roughly two hours: certificate verification, 403, connection reset, timeout, and now another timeout. @CozySocksAlways mentioned on #708 they were going to look into the docs side, so flagging it here for the same reason.

@coveralls

Copy link
Copy Markdown
Collaborator

Coverage Status

coverage: 45.709% (-42.9%) from 88.605% — adityasingh2400:fix/instantaneous-rate-sampling-period-zero into NeuralEnsemble:master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants