Apply the border correction before pooling spike trains in instantaneous_rate - #708
Conversation
instantaneous_rate(border_correction=True, pool_spike_trains=True) raised IndexError. Pooling collapsed the column axis of the rate to a single column, and the border correction that runs afterwards loops over all input spike trains and rescales rate[:, i] per spike train, so every column past the first was out of bounds. The correction is defined per spike train, it rescales a column so that the integral over it returns the spike count of the matching spike train, which means it has to run before the columns are averaged. Correcting first also keeps the documented meaning of pool_spike_trains, 'firing rates averaged over spike trains', because the pooled rate is then exactly the mean over the columns of the pool_spike_trains=False result. It matches the order already used for elephant.trials input, where the trials branch computes the per spike train rates with pool_spike_trains=False and averages the corrected result afterwards.
The same spike trains wrapped in an elephant.trials object already return the mean of the border corrected per spike train rates, because the trials branch passes pool_spike_trains=False down and averages afterwards. Assert that the list input now agrees with it exactly.
|
The docs jobs here are failing on a remote fetch, not on this branch. The sphinx build dies executing a notebook that pulls sample data over HTTPS, ending in The other elephant PR I have open, #706, is red on the same theme from the other direction, with |
|
Following up on my note above, which only covered the docs job. The On the latest run here the only failures are Worth flagging one knock-on effect: Coveralls reports coverage dropping to roughly 45.7 percent from 88.6. That looks alarming but is consistent with the failing jobs never uploading their coverage, leaving only a partial report merged. I would not read it as a real coverage regression from these changes. |
|
Hello :) |
|
Thanks, and good to have the coverage read confirmed rather than assumed. No rush on the review. I will leave the three of them alone rather than pushing rebases at them, since nothing on these branches will move those particular jobs. One small data point for the docs failure if it helps when you dig in: across the three PRs that fetch has now failed four different ways in about an hour, a certificate verification error, a 403, a connection reset, and a timeout. That spread looks more like the host refusing or throttling than a single dependency behaving badly, though it could of course be both. |
|
Thanks, that all matches what I found. On the docs failure, I had already chased that one down before your message and opened #709 for it, so you may not need to spend time on it. Short version: Raising the matplotlib cap does not work, which I only found by trying it. viziphant 0.4.0 is the latest release and still calls Happy to close #709 if you would rather fix it a different way, it is a two line change. |
|
I managed to pull the full job logs for all five of my open PRs here, so this is now measured rather than inferred. Posting it on this one since the picture is the same across all of them.
Every failure in all five is in The part I find most telling is that the count differs between runs, 1 through 3, and the set of failing tests differs too, even though none of these branches touch the download path. Same code, different tests failing each time, which is what a flaky remote fetch looks like rather than a real regression. None of the five needs a change from me as far as I can tell. Happy to be told otherwise if you see something in there I am reading wrong. |
|
Correcting one line in my last comment and replacing it with better evidence. I wrote "same code, different tests failing each time". That was sloppy, those are five different branches, so it is not the same code and the variation across them proves less than I implied. Here is the check that does settle it. Same test class, same SSL error, same shape as all five of my PRs, on your own default branch. So this is not something any of the branches introduced, and the varying subset across runs is consistent with which downloads happen to fail on a given attempt. The rest of what I posted stands: every failure in all five PRs is confined to |
|
One more data point, and it makes this cheaper to clear than my last comment implied. The Canary failures are intermittent rather than a standing break. On All five of my open PRs last ran CI on 2026-08-05 between 15:47 and 17:33, inside that failing window, and none has re-run since. So a re-run is plausibly all they need rather than a fix to the certificate handling. I do not have permission to trigger one. |
|
Thanks for the re-run. It cleared two of the four, The two that remain are separate problems.
That is exactly what #709 pins against, and
One lead on that rather than a diagnosis, since I cannot reproduce the 403 from outside CI. That URL is built from the package version, which is |
|
Correcting my last comment. I said the Within that run, on the same commit and within three minutes of each other: 774 is 769 plus 5, so I do not have a cause beyond that, so I will not guess at one. |
Combining the two documented flags
border_correction=Trueandpool_spike_trains=Trueon a list of spike trains raisesIndexError.On current master:
Pooling runs first and collapses the column axis to a single column, then the border correction loops over every input spike train and rescales
rate[:, i], so every column past the first is out of bounds.The order is the bug, not the indexing. The correction is defined per spike train, and its second half rescales a column so that the integral over it returns the spike count of the matching spike train, so it has to run while the columns still correspond one to one with the spike trains. Correcting first also preserves the documented meaning of
pool_spike_trains, "calculate firing rates averaged over spike trains", because the pooled rate is then exactly the mean over the columns of thepool_spike_trains=Falseresult, an invariant that already holds whenborder_correction=False.This is not a judgement call, the same order is already in the file. For an
elephant.trialsinput the trials branch passespool_spike_trains: Falsedown and averages the corrected per spike train rates afterwards. The two routes disagree today: with the same four spike trains and the same flags, wrapping them inTrialsFromListsworks while passing them as a plain list raisesIndexError. After this change both return the same array, with a max absolute difference of 0.0, which the test now asserts.Behaviour on inputs that already worked is unchanged. With
border_correction=False, pool_spike_trains=Truethe output is bit identical to master for bothtrim=Falseandtrim=True, including shape,t_start,sampling_periodand the kernel annotation.The new test checks the output shape
(n_bins, 1), equality with the column mean of the un-pooled result, that the integral over the pooled rate equals the mean spike count, and agreement with the trials path.Reverting
statistics.pyto master fails with theIndexErrorabove. After, 102 passed, 1 skipped and 33 subtests passed.pycodestylereports no new findings. The 5 pre-existingstatistics.pydoctest failures are identical on master and on this branch, consistent with known issue #686.I have not added myself to
doc/authors.rst, since it is a numbered institutional affiliation list. Happy to add an entry if you would like one.Disclosure: this change was prepared with AI assistance. I have reviewed and tested it.