Clip small negative values in van_rossum_distance before np.sqrt #680
Conversation
…rom np.sqrt; added test case for same
|
|
||
|
|
||
| # Check small negative values edge case | ||
| with warnings.catch_warnings(record='True') as w: |
There was a problem hiding this comment.
The expected type for argument record is bool, not str
| self.assertEqual(len(stds.van_rossum_distance([], self.tau3)), 0) | ||
|
|
||
|
|
||
| # Check small negative values edge case |
There was a problem hiding this comment.
Although there is a single unit test for the function, it is clearer to add a specific regression unit test for the issue. We have examples in other modules, e.g., test_instantaneous_rate_regression_288 in elephant.statistics. It is good to point to the issue number and have a short comment.
| self.st23 = StationaryPoissonProcess(rate=30 * Hz, t_start=0 * ms, | ||
| t_stop=1000 * ms | ||
| ).generate_spiketrain() | ||
| self.st24 = SpikeTrain([0.1782, 0.2286, 0.2804, 0.4972, 0.5504], units='s',t_stop=4.0) |
There was a problem hiding this comment.
As suggested by the dedicated regression test, the spike train can be initialized locally in the function, which facilitates code reading.
| # Clip small negative values | ||
| if np.any(vr_dist < 0): | ||
| warnings.warn( | ||
| "van_rossum_distance: very small negative values encountered " |
There was a problem hiding this comment.
Here, it would also be helpful to hint at possible causes. As we tested, this does not happen if the units are in ms, and the user could change the representation to prevent the warning if noticed. Maybe something like "... very small negative values encountered, setting them to zero. This is likely due to floating point error, which can happen if the spike times are small floating point values. In this case, a possible solution to prevent the warning is to change the time unit of the input spike trains for better precision (e.g., from seconds to milliseconds).".
|
The implementation looks good. I made small suggestions to improve the warnings and structure of the unit test. |
kohlerca
left a comment
kohlerca
left a comment
There was a problem hiding this comment.
I reviewed the recent changes, and they look good to me. The PR can be merged.
4 participants
This PR fixes Issue #679 . Currently
van_rossum_distancecould returnnandue to extremely small negative values because of floating point errors before applyingnp.sqrt.Fix
Added check before
np.sqrtto find small negative values, then replaced them usingnp.maximum(vr_dist,0). Also outputs runtime warning for transparancy.This ensures that spike trains that should result in a distance of 0 no longer produce
nan.Test
Added unit test that reproduces the spike train scenario from the issue, it asserts that the computed distance is 0 and not
nan