-
Notifications
You must be signed in to change notification settings - Fork 240
Fix get_traces for a local common reference #2649
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix get_traces for a local common reference #2649
Conversation
|
Hi Chris, does it fail if you call the |
Morning! It works for |
| def test_common_reference_select_channels_local(recording): | ||
|
|
||
| recording_cmr = common_reference(recording, reference="local") | ||
| recording_segment = recording_cmr._recording_segments[0] | ||
|
|
||
| traces_all = recording_segment.get_traces(start_frame=0, end_frame=10, channel_indices=[0, 1, 2, 3]) | ||
| traces_sub = recording_segment.get_traces(start_frame=0, end_frame=10, channel_indices=[1, 3]) | ||
|
|
||
| assert np.all(traces_all[:, [1, 3]] == traces_sub) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chrishalcrow instead of this "segment" test, I guess you can trigger the same issue by adding this line:
local_trace = recording_local_car.get_traces(channel_ids=["c", "d"])
after L71
I would actually prefer to extend that test instead of adding this new one, since it was designed (badly) to test the slicing behavior. Could you do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have extended the test to include what you say, and also slicing the segments for each type of common reference. Also made the tests a little harder by choosing ["b", "d"] rather than ["a", "b"]. I also shorted the generated recording from 5s to 1s: I can't see any reason here not to do this and it doubles the speed of the test for me locally.
alejoe91
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thansk @chrishalcrow
Check comment!
| re_referenced_traces = np.zeros_like(traces[:, channel_indices]) | ||
| channel_indices_array = np.arange(traces.shape[1])[channel_indices] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even theses 2 lines looks very very bad. no ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is because channel_indices can be a slice and we need an iterator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe they look bad, but they're not breaking anything! Is this ultimately due to the difference between channel_ids and channel_indices?
Co-authored-by: Alessio Buccino <alejoe9187@gmail.com>
Fixes bug in
get_tracesfor common reference recordings with alocalreference, where you couldn't select a subset of channels. Noticed it when plotting traces.Here's minimal code which reproduces the bug
This should raises an error.
Also added a test which selects a subset of channels ids, and makes sure it's selected the right one.