-
Notifications
You must be signed in to change notification settings - Fork 249
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 electrode selection in Axona raw recording #1520
base: master
Are you sure you want to change the base?
Fix electrode selection in Axona raw recording #1520
Conversation
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 looks pretty good to me. The failing test seems to be a ci issue and not related to this PR so I'll try to figure that out. Let me know what you think about the comments :)
neo/rawio/axonarawio.py
Outdated
cntr = (itetr * elec_per_tetrode) + ielec | ||
ch_name = f"{itetr + 1}{letters[ielec]}" | ||
cntr = (itetr * elec_per_tetrode) + ielec + first_channel | ||
ch_name = "{}{}".format(itetr + active_tetrode_set[0], letters[ielec]) |
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.
ch_name = "{}{}".format(itetr + active_tetrode_set[0], letters[ielec]) | |
ch_name = f"{itetr + active_tetrode_set[0]}{letters[ielec]}" |
we try to keep the codebase on f-strings since they are faster and easier to read :)
neo/rawio/axonarawio.py
Outdated
@@ -558,6 +557,20 @@ def get_active_tetrode(self): | |||
tetrode_id = int(key.strip("collectMask_")) | |||
active_tetrodes.append(tetrode_id) | |||
return active_tetrodes | |||
|
|||
def get_active_channels(self): |
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.
public or private? what do you think?
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 advice to make things private by default and move away from it when you have a good reason to.
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.
That's how I was leaning too. I don't see why the user would run this function.
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.
Yes, I agree with making them private
chan = self._get_channel_from_tetrode(tetrode) | ||
active_channels.append(chan) | ||
|
||
return np.concatenate(active_channels) |
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.
what's the benefit of concat? I need to doublecheck what is chan
is it a list of all channels from a tetrode. If that is the case I would call it channels
or chans
instead so we know it's plural.
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.
Yes, chan
is a list of all channels from a tetrode, so I agree to call it channels
or chans
.
Since I was using this function only for the _get_analogsignal_chunk
function and it needed the list of active channels as an array I returned directly active_channels
as an array. But we can also do it in _get_analogsignal_chunk
and keep this function consistent with get_active_tetrodes
and return directly active_channels
@alejoe91 do you know this datalad error? it only happened for one dataset on the 3.9 python vs the 3.11 python runs. It's completely unrelated to this PR, but if you recognize the issue then I won't spend time digging into it. |
@zm711 maybe just a random failure, re-triggered to see what happens! |
I tried retriggering once, but yeah let's see if maybe it was just a timing issue. EDIT; --Yep okay just a random ci failure. cool cool |
Gin data for this? : O |
You mean you want extra gin data? This was a bug in the original implementation just because we didn't know which channel was which. If you use the current gin data with the spikeinterface extractor you'll see that the channels are just 1-16 or 1-32 rather than being meaningful. |
Thanks. I did not realize we already had the gin data as I came from the neuroconv trail. |
@letiziasignorelli just wanted to see if you'd have bandwidth to work on this before the end of the year :) ? Otherwise I'll change the tag to |
@zm711 I replied to all comments, and from my side I was just waiting for approval :). Is there anything I should still work on/change? |
Did you forget to push the changes? I don't see any of the changes--(I see your comments, but not a commit where you have the changes). Once they are pushed here then the last thing is just resolving the conflict in the author doc :) |
Okok you're right, I'll push the changes in the next few days. Thanks :) |
@@ -377,8 +377,7 @@ def _get_analogsignal_chunk(self, block_index, seg_index, i_start, i_stop, strea | |||
if channel_indexes is None: | |||
channel_indexes = [i for i in range(bin_dict["num_channels"])] | |||
elif isinstance(channel_indexes, slice): | |||
channel_indexes_all = [i for i in range(bin_dict["num_channels"])] | |||
channel_indexes = channel_indexes_all[channel_indexes] | |||
channel_indexes = self._get_active_channels() |
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.
@h-mayorquin do you want to doublecheck this. I'm just re-reading and I'm thinking of the case of a channel slice so would love another pair of eyes on the logic here.
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 am confused on why this function does not take any input, I would expect this to use the slice (channel_indexes) that it was passed?
I'm proposing this PR to fix #1454
@zm711