-
Notifications
You must be signed in to change notification settings - Fork 261
Refactor plexon rawio to have same ids as plexon2 #1547
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
Refactor plexon rawio to have same ids as plexon2 #1547
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.
Thanks Heberto one question and then two tiny fixes to make.
neo/rawio/plexonrawio.py
Outdated
self.stream_id_samples = {} | ||
self.stream_id_sampling_frequency = {} | ||
self.stream_index_to_stream_id = {} |
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.
the general idea of these attributes was that the equivalents were private before. Why switch to public in general?
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.
Good point. I think I just copy pasted from plexon2 but they should be private in both sides.
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 made them private in the two places.
https://github.com/NeuralEnsemble/python-neo/actions/runs/10776574774/job/29883638305?pr=1547 I just want to save this failed attempt with 15 times. This is good to know since I haven't seen the runner fail since you implemented the 10 retries. This is interesting. |
Oh, what a pain : / |
Agreed. I think explicitly setting this is probably a better idea rather than setting trying until success. So I don't know, if we set something big like 100 I don't want to slow down the test suite. Maybe we hive off plexon2 as a conditional test--only when baserawio, toml, or plexon2 changes. Any opinions? |
Let's measure how much time it takes. |
@zm711 This is ready. I measured the opening on my computer for a file of around 2 GiB and it took around two milliseconds: 2.46 ms ± 58.6 μs per loop (mean ± std. dev. of 7 runs, 100 loops each) import platform
from urllib.request import urlopen
import pathlib
architecture = platform.architecture()[0]
if architecture == "64bit" and platform.system() in ["Windows", "Darwin"]:
file_name = "PL2FileReader64.dll"
else: # Apparently wine uses the 32 bit version in linux
file_name = "PL2FileReader.dll"
pl2_dll_folder = pathlib.Path.home() / ".plexon_dlls_for_neo"
pl2_dll_folder.mkdir(exist_ok=True)
pl2_dll_file_path = pl2_dll_folder / file_name
if not pl2_dll_file_path.exists():
url = f"https://raw.githubusercontent.com/Neuralensemble/pypl2/master/bin/{file_name}"
dist = urlopen(url=url)
with open(pl2_dll_file_path, "wb") as f:
print(f"dll file does not exist, downloading plexon dll to {pl2_dll_file_path}")
f.write(dist.read())
from neo.rawio.plexon2rawio.pypl2.pypl2lib import PyPL2FileReader
pl2reader = PyPL2FileReader(pl2_dll_file_path=pl2_dll_file_path)from pathlib import Path
file_path = Path("/home/heberto/Downloads/1_conspecific_vocalization_block_10.16.23.pl2")
assert file_path.exists()
filename = str(file_path)
# In another cell
%%timeit
pl2reader.pl2_open_file(filename) |
Cool, But seems like you didn't change the open tries for plexon2? I'm just wondering if we should change that (in a new PR?). Let me double check this PR one last time after my morning meeting and then we can merge. I'm just going to update the branch first. |
I just changed them. |
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.
Looks good to me, note that we increased the attempts because this is failing on CI sometimes likely due to a wine issue for the CI runner.
Since we are meeting tomorrow I'll wait for our meeting to merge.
OK. |
After #1541 Plexon2 ended up with the meaningful but unique stream_ids:
But in Plexon 1 we are using numbers instead: "1", "2", ...
This PR unifies them so they have the same stream_ids