Axon: improve channel name handling#1874
Conversation
zm711
left a comment
There was a problem hiding this comment.
I know your argument is that we should return them correctly, but is there really no way for us to think about backward compatibility for people who used to use Neo. I would probably tend to agree with you and that people just need to adapt their previous Neo code to fit with the machine convention, but just trying to brainstorm.
I have been thinking about this, not only with channel names but with stream ids as well. Thanks for making me overcome the energy barrier and write this down here: Check it out, I have a concrete proposal so we can move forward with changes like this and also solve old issues regarding the contract of the API. I would like to hear your thoughts. |
|
I think with your proposal I'm okay with going forward with this and once we have approval from the group we can think about implementing this translation layer. |
| # byte can never crash the read. | ||
| if version < 2.0: | ||
| name = info["sADCChannelName"][chan_id].replace(b" ", b"") | ||
| name = info["sADCChannelName"][chan_id].decode("utf-8", errors="replace").strip() |
There was a problem hiding this comment.
One last question. I remember on the spikeinterface side we had some formats that failed with 'utf-8' decoding. Is there a failure possibility here using this type of decode? Or the replace guarantees this will always work?
There was a problem hiding this comment.
So, errors="replace" avoids the raise. Instead of throwing UnicodeDecodeError on invalid bytes, it substitutes them with the � replacement char, so the decode can never crash the read no matter what's in the field. The strict default (decode("utf-8") with no errors=) would raise.
What do you think?
There was a problem hiding this comment.
I'm just wondering if raising an error is important in this case for the regular end-user? If this can't be UTF-8 is that a signifier that should not be silently glossed over? Again I don't use this format at all, so I can't say if replace or not is the better option.
I think from your perspective you'd prefer not to crash so you can inspect the file and decide for yourself whether it is okay or not. But for the non-power user is there a benefit to not even loading poorly formed formed channel names?
There was a problem hiding this comment.
Yes. I think it is better that they get ugly channel names rather than not being able to load their data. But I am not sure this will ever happen. This is kind of edging against axon encoding badly for some reason.
zm711
left a comment
There was a problem hiding this comment.
Sounds good. I'm okay moving forward with this and we can back port backward compatibility if we decide to move forward with that.
Two improvements to the channel names in
AxonRawIO:Names are decoded to
strwith their real content preserved. They were previously kept asbytesand space-stripped with.replace(b" ", b""), which also destroyed interior spaces, so"IN 1"came back as"IN1". Now only the fixed-width padding is stripped, interior spaces are kept, and decoding useserrors="replace"so an odd byte can never crash the read.A channel whose stored name is blank now falls back to a positional
ch{id}instead of an empty""that left the channel unaddressable.The protocol path (
sProtocolPath) is decoded tostrthe same way, since as raw bytes itsstr()baked a"b'...'"literal into the path.One intended behavior change: files whose channel names contain interior spaces now report the real name, for example
File_axon_6andFile_axon_7return"IN 1"instead of"IN1". The space is part of the name Clampex recorded, so this is the correct value; any downstream code that matched the old space-stripped form should use the real name. The blank-name case is covered by the fixtureabf1_episodic_empty_channel_name.abf.This is also ready @zm711