Fix-ate plexon 2 streams #1541
Fix-ate plexon 2 streams #1541
Conversation
|
|
||
| self.pl2reader = PyPL2FileReader(pl2_dll_file_path=pl2_dll_file_path) | ||
|
|
||
| reading_attempts = 10 |
There was a problem hiding this comment.
Is it possible that you have a good attempt in the middle and bad attempt at the end of the range that then messes up the attempts such that you just have to guess when to end on a "good attempt". Ie is there a smarter way for us to check like
for i in range(num_attempts):
read
if read is good
break
There was a problem hiding this comment.
That's how it is right now. If the handle is good you exist the loop:
python-neo/neo/rawio/plexon2rawio/plexon2rawio.py
Lines 139 to 141 in 221befa
In other words, it is first success out.
There was a problem hiding this comment.
sorry I missed that line. Sorry didn't expand the diff.
There was a problem hiding this comment.
It is a good suggestion though!
There was a problem hiding this comment.
And so it silently fails? We have yet to see any stochastic failures of our test files so far. It's just so weird for it to fail. Has plexon commented on this?
There was a problem hiding this comment.
You did at the beginning before we made this patch:
The re-attempt mechanism solves most of the case almost all of the time except when it does not. For some reason it has been failing for neuroconv's docstring tests.
The guy that I am discussing with is from Plexon.
There was a problem hiding this comment.
I meant test failures since your PR. It seems like the 10 tries has always worked for us thus far. It seems weird that the flakiness is so flaky, but yeah based on the discussion with plexon there they also seem to be a bit confused by this issue. So I'm fine with just letting the person try as many times as they want.
There was a problem hiding this comment.
Yeah, it is kind of fishy that it always fails in our doctest. But if I discover that that indeed is the case that more attempst solve the problem and that different envs require different attemps I don't want to have to create a new PR every time so better exposed it.
It is unlikely that we find a root case solution if the plexon people also are suprised about it because it is probably a wine dllc related problem.
There was a problem hiding this comment.
This looks pretty good to me. @samuelgarcia or @alejoe91 want to take a quick scan of this one?
Co-authored-by: Zach McKenzie <[email protected]>
| signal_streams = np.array(signal_streams, dtype=_signal_stream_dtype) | ||
|
|
||
| self.stream_id_samples = {} | ||
| self.stream_index_to_stream_id = {} |
There was a problem hiding this comment.
Why this ?
stream_index = header["signall_streams"]["id"][stream_index] should work no ?
There was a problem hiding this comment.
To be honest I think Heberto and I like the slightly more explicit variable names in order to quickly know where to get things. it's tricky to remember the complicated header structure sometimes so the separate variables just make it a little more explicit for the steps.
There was a problem hiding this comment.
Yeah, I think that could work as well it just did not occur to me. I don't have a strong preference here, Sam. As zach mentions I have a preference for explicit but in this case I just came with this idea first. If you prefer I can eliminate it.
There was a problem hiding this comment.
OK. No worry. Just curious.
|
I think we should merge #1546 first and then incorporate it's bug fix into this PR. |
Seems that there were no conflicts. |
Cool. I didn't look too closely to see if you had touch those specific lines or not. So I just wanted to check that before. We can wait for Sam to decide if you should user the header instead of the dict you made and then we can merge after he decides on that. |
|
This is OK for me. Can I merge ? |
3 participants
Same as #1524 but for Plexon 2.
Also, I am exposing the number of retries as that is non-deterministic in plexon2 and that has been failing in some our CI in neuroconv because the number is too small.