[Fix] instantaneous rate with list[neo.SpikeTrain] #649
Conversation
Moritz-Alexander-Kern
changed the title
…lace raise error with return False
kohlerca
left a comment
kohlerca
left a comment
There was a problem hiding this comment.
I checked the code and made suggestions. Regarding the proposed solution, this needs to be investigated further to ensure the problem was only due to a wrong class membership check. According to the comment on line 1034, the check that originated the error might be misplaced and redundant.
|
|
||
| from elephant.trials import Trials | ||
|
|
||
| import collections.abc |
There was a problem hiding this comment.
from collections.abc import Iterable. Improved readability and reduced number of elements in the namespace.
There was a problem hiding this comment.
It also avoids always loading additional objects with attribute statements each time is_list_spiketrains is executed.
| # Input must be an iterable (list, tuple, etc.) | ||
| return False | ||
|
|
||
| if not all(isinstance(st, neo.SpikeTrain) for st in obj): |
There was a problem hiding this comment.
This can be replaced by a direct return statement:
return isinstance(obj, Iterable) and all(isinstance(st, neo.SpikeTrain) for st in obj)
AND statements are evaluated lazyly from left to right. Whenever an non-Iterable is passed, the expression will be False and evaluation stops.
| Returns | ||
| ------- | ||
| bool | ||
| True if obj is an iterable containing only neo.SpikeTrain objects. A single `neo.SpikeTrain` object (not inside |
There was a problem hiding this comment.
Formatting not conforming to the style guide:
obj(between backticks) to highlight the function parameter;neo.SpikeTrain(between backticks) objects- False without backticks
Other PRs are explicitly adding links to the documentation of the referred classes using Sphinx's :class: statements, which facilitate navigating through the documentation.
|
|
||
| def is_list_spiketrains(obj: object) -> bool: | ||
| """ | ||
| Check if input is an iterable containing only neo.SpikeTrain objects. |
There was a problem hiding this comment.
Corrections:
- ... if the input ...
- highlight
neo.SpikeTrainusing backticks or the class reference
|
|
||
| if isinstance(spiketrains, neo.core.spiketrainlist.SpikeTrainList) and ( | ||
| pool_spike_trains): | ||
| if is_list_spiketrains(spiketrains) and (pool_spike_trains): |
There was a problem hiding this comment.
Parentheses around pool_spike_trains not needed. Also, the order in the condition should be
if pool_spike_trains and is_list_spiketrains(spiketrains):
This avoids unnecessary calls to the function if not pooling (lazy evaluation of the AND statement).
There was a problem hiding this comment.
However, analyzing the function flow, the new and old checks for a list of neo.SpikeTrain objects may not be needed at all:
- in lines 908-911 it is ensured that there will be an Iterable with at least one spike train
- in lines 913-916 an error will be generated if the input is not an iterable that contains only spike trains
Therefore, at this point, it would be possible to only check if pooling or not the spike trains.
| pool_spike_trains=False, | ||
| pool_trials=True) | ||
| self.assertIsInstance(rate, neo.core.AnalogSignal) | ||
| self.assertEqual(rate.shape[1], self.trial_object.n_spiketrains_trial_by_trial[0]) |
There was a problem hiding this comment.
This test checks for consistency between the dimensions of the computed rate and the data in the Trials objects. However, an additional test against the integer values would protect against errors in implementing the object attributes. These hard expected values are supposed to be known when designing the test data in line 493.
| pool_spike_trains=True, | ||
| pool_trials=False) | ||
| self.assertIsInstance(rate, list) | ||
| self.assertEqual(len(rate), self.trial_object.n_trials) |
There was a problem hiding this comment.
Same comment as above, where the test does not compare directly to the expected values, but retrieves them dynamically from the objects.
| pool_spike_trains=False, | ||
| pool_trials=False) | ||
| self.assertIsInstance(rate, list) | ||
| self.assertEqual(len(rate), self.trial_object.n_trials) |
There was a problem hiding this comment.
Same comment as above, where the test does not compare directly to the expected values, but retrieves them dynamically from the objects.
| pool_trials=False) | ||
| self.assertIsInstance(rate, neo.core.AnalogSignal) | ||
| self.assertEqual(rate.magnitude.shape[1], 2) | ||
| self.assertEqual(rate.magnitude.shape[1], self.trial_object.n_spiketrains_trial_by_trial[0]) |
There was a problem hiding this comment.
Same comment as above, where the test does not compare directly to the expected values, but retrieves them dynamically from the objects. The integer in the old test could be updated, and the new one just added as an additional check.
| self.assertEqual(rate.shape[1], self.trial_object.n_spiketrains_trial_by_trial[0]) | ||
|
|
||
| def test_instantaneous_rate_list_pool_spike_trains(self): | ||
| def test_instantaneous_rate_trials_pool_spiketrains(self): |
There was a problem hiding this comment.
A suggestion to improve understanding of these unit tests, where the dimensions of the outputs change depending on the pooling, is to include a comment block at the beginning to explicitly state the input dimensions --> expected output dimensions. It would be easier to assess the behavior of the object and what is being tested in each test case.
3 participants
The pull request addresses inconsistent behavior with different options in combination with various inputs, specifically:
Issue:
pool_spiketrains = Truewithlist[spiketrains]does not pool spiketrains as expected. The output should be a singleneo.AnalogSignal.Fix
neo.spiketrainlistwas regarded