[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.
elephant/statistics.py
Outdated
|
|
||
| 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.
elephant/test/test_statistics.py
Outdated
| 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.
elephant/test/test_statistics.py
Outdated
| 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.
elephant/test/test_statistics.py
Outdated
| 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.
elephant/test/test_statistics.py
Outdated
| 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.
4 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