MaxwellIO: Fix issue due to disparity between routed and mapped channels #1703
Conversation
Update mask to only allow unique electrodes to pass in addition to filtering out non-routed electrodes
There was a problem hiding this comment.
Thanks for the contribution @LeMuellerGuy!
I have a couple more stylistic comments to go through. And also a general preference to use channel_ids and electrode_ids here in the function to be explicit as possible.
Otherwise I ran testing so we will make sure this current iteration doesn't break on our test files.
Is there anyway you can generate a really small test file (<10MB that can be junk data) that we could add to our test suite to make sure your fix works?
| @@ -65,6 +65,19 @@ def __init__(self, filename="", rec_name=None): | |||
|
|
|||
| def _source_name(self): | |||
| return self.filename | |||
|
|
|||
| def _get_ids_and_electrodes(self, version, stream_id, h5file, mapping): | |||
| if int(version) == 20160704: | |||
There was a problem hiding this comment.
Could you document where this version number comes from in a comment? Was this dev docs etc. I think knowing magic numbers at all times.
There was a problem hiding this comment.
I took the version number from the existing code. My best guess is that it is actually the release date of the first file version, where the structure was a little different. Our lab works with the MaxTwo device which was released later on so I assume that it never used this version because it needs additional fields to correctly represent the multiple wells of the MaxTwo device. There is also an option for legacy data formats in the software supplied by maxwell, so I assume it must be that.
neo/rawio/maxwellrawio.py
Outdated
| else: | ||
| ids = np.array(h5file["wells"][stream_id][self.rec_name]["groups"]["routed"]["channels"]) | ||
| els = np.array(mapping["electrode"]) | ||
| ids = ids[ids >= 0] |
There was a problem hiding this comment.
what does an id < 0 mean in this case ?
I see we do the same thing in the old code, but since I don't work on this reader much I would love to know what this means.
There was a problem hiding this comment.
This was also taken from the existing code and I can only guess what it means. I know that the spike files can contain negative timestamps relative to the starting time so I wouldn't put it beyond them to also have negative channel numbers in some scenarios, but I personally haven't encountered them yet. Maybe that is also something occuring in the older data format.
neo/rawio/maxwellrawio.py
Outdated
| ids = np.array(h5file["wells"][stream_id][self.rec_name]["groups"]["routed"]["channels"]) | ||
| els = np.array(mapping["electrode"]) | ||
| ids = ids[ids >= 0] | ||
| mask = np.unique(mapping["channel"][np.isin(np.array(mapping["channel"]), ids)], |
There was a problem hiding this comment.
I would prefer to break this up into a couple lines because it is really hard to follow the logic when everything is chained together.
There was a problem hiding this comment.
It's not highlighting the line I wanted it to. I meant for line 77 it is really chained logic.
There was a problem hiding this comment.
Totally understandable, I tried to split it into two. Maybe you have a more expressive variable name for the masks created.
zm711
changed the title
|
The PR name change was just because it is easier for me to make our release notes if I can quickly glance to see which IOs have been worked on :) |
Stylistic cleanup
|
Hi Zach, You're also on the probeinterface team right? Because I noticed that this issue propagates to probeinterface as well so I'll open a similar PR there too. I'll try and "filet" a piece out of the dataset. I think it should be possible. |
|
Hey again, I created a data slice that basically represent a single well, single segment recording that contains the mismatch between routed channels and mapped channels. It's only 100 datapoints long, but I can make it longer if you like. |
|
Hey again, I had the time to do some further investigation on my file. It seems to be the case that the issue always takes the form that two channels can be mapped to the same two electrodes. There can also be more than one pair of these duplicates per recording segment. Initially I thought that this would be physically impossible for the device but after looking into the data, I am not quite sure about this anymore so it is be possible that the mapping file is indeed correct. I'm not sure how neo would navigate a surjective mapping of channels to electrodes since that is usually not a very useful situation. I'll do some further digging once I get back to the lab tomorrow but for now I'd put this PR on ice until I found the source of this issue in the device and can give more insight into how to navigate the situation. Maybe this results in a more meaningful error message for future users. |
|
Sounds good @LeMuellerGuy thanks for checking on that. It does seem a bit strange unless it really is a recording electrode and a stimulating electrode being associated with the same channel. But even then we would expect the need to unpack the two different information streams in order to be useful. Just ping us when you have some more info! |
|
@philipp-mxw, maybe you would be good to comment on this PR? Whenever you have a moment, just so we can better understand some of this mapping? |
|
Sorry for the late reply, I was away for a few days. Due to the flexibility of the switch matrix design, it is possible to configure it such that one electrode connects to multiple readout channels, or multiple electrodes share the same readout channel. This can occur unintentionally during stimulation routing, e.g. if two stimulation electrodes are routed to the same stimulation unit. In such cases, the electrodes become electrically connected via the stimulation unit, and the mapping (correctly) reports that both electrodes are routed to the same set of readout channels. The MxW H5 mapping structure does not support a mapping of one readout channel to a list of electrodes. Instead, one-to-many mappings appear as multiple rows in the mapping table. One for each channel-electrode pair. If the stimulation is meant to activate both electrodes simultaneously, this setup works fine, as the stimulation unit can drive multiple electrodes. However, if independent stimulation is required, this configuration would not be valid due to the shared connection and the routing must be changed by the user, for example by selecting different electrodes. We are currently working on improving the routing logic to ensure each stimulation unit is only connected to one electrode, which will prevent such routing conflicts in the future. For practical purposes when loading the mapping from file, it is reasonable to ignore cases where multiple readout channels connect to the same electrode (or electrode set), as this typically has no functional impact, unless they are shared due to a common stimulation unit. In this case the data will only show the voltage that the voltage stimulation unit applies at the electrode. However, cases where multiple electrodes share the same readout channel should be preserved if possible, as they reflect actual electrical connections and may affect data interpretation. For example, in the H5 file attached to this PR, the mapping is as follows: Channel 399 -> electrodes [25732 13364] Channel 399 -> electrodes [25732 13364] I know that was a lot, so please let me know if that was clear enough or you have any more questions! |
|
Thanks for the clarification. I was already suspecting that it may be due to the stimulation buffers. This might be a little off topic but if I understand it correctly there is not really a way to prevent the two electrodes being connected to the same stimulation buffer is there? To the user it's basically just chance how the connection ends up after routing, since you can only query the stim buffer connections and not set them. Nevertheless, it seems quite unfortunate since I am not sure if neo, probeinterface, and spikeinterface are currently set up for a one to many mapping of device channels, which essentially makes these kind of files unusable in this context. It's probably also unreasonable to expect to change this since it would require rather hefty changes while the phyisological relevance of a two electrode one channel setup is probably questionable in most common applications. |
|
Yes, unfortunately, there is currently no way to avoid two electrodes from being connected to the same stimulation unit. However, we are aware of this issue and are working on implementing changes to prevent this from happening. For the time being, it is probably best to just remove any duplicates in order to be compatible with neo/spikeinterface etc. since it will most likely either contain the stimulation voltage or a mixed signal from two electrodes. |
As mentioned in #1702, there can be a potential disparity between the list of mapped channels and routed channels when using stimulation electrodes. This PR attempts to fix this by ignoring the additional electrodes since they have no data associated to them/ neo can currently only pull data from the routed group. I don't know if there can be additional groups in the data though.