NeuralynxIO: support List[str] for filename parameter #1440

NxNiki · 2024-03-19T20:10:20Z

Add a feature to allow multiple files as filename.

Now user can create a list of filenames and assign it to the argument include_filenames of neo.io.NeuralynxIO(dirname='xxx', include_filenames=session_data) to read the specific files.

If there are multiple files for a single channel, they should be consistent for all channels.

zm711 · 2024-03-20T11:27:26Z

I guess one question I would have overall is why overload the filename argument? Unfortunately I don't think we would change (like change filename->filenames) since it is a core part, but why not add another argument include_filenames to mirror the exclude_filenames. I'll ping @samuelgarcia here so he can decide if he is okay with the overloading filename or not.

samuelgarcia · 2024-03-20T12:47:08Z

Maybe w could use filenames to avoid confusion ? even if it makes the signature a bit heavy...

zm711 · 2024-03-20T12:57:45Z

I think having filename and filenames would be super confusing and misused a lot. Would you be okay with include_filenames then it would still be dirname='path', filename=None, and include_filenames=['file0', 'file1']?

Or are you suggesting switch filename to filenames and have some broken code?

NxNiki · 2024-03-20T17:48:32Z

I think having filename and filenames would be super confusing and misused a lot. Would you be okay with include_filenames then it would still be dirname='path', filename=None, and include_filenames=['file0', 'file1']?

Or are you suggesting switch filename to filenames and have some broken code?

I don't feel it is necessary to create separate variables for single and multiple input files in the arguments. Maybe we can use filenames as the argument and create self.filename = filenames if filenames is a string and do self.filename = filenames[0] if len(filenames)==1?

The local filenames is also created when dirname is used. I don't feel using it for multiple input files adds more confusion here.

zm711 · 2024-03-20T17:53:26Z

@NxNiki,

I agree that's the better overall solution, but changing the argument name could break users' code. So my question for Sam was whether he was okay to do a breaking change at the api level or if he wanted to work around the current api to protect the old way of doing things.

NxNiki · 2024-03-20T17:57:46Z

@NxNiki,

I agree that's the better overall solution, but changing the argument name could break users' code. So my question for Sam was whether he was okay to do a breaking change at the api level or if he wanted to work around the current api to protect the old way of doing things.

Maybe add a deprecate warning?

apdavison · 2024-03-20T22:48:55Z

I think this is perhaps an opportunity to formalize IO constructor arguments across all IO classes, before we get to the 1.0 release.

I think it will be confusing to have some IO classes that accept a list of things as the first argument, and others only a single thing. To solve the current issue, I support @zm711's suggestion of adding include_filenames, but then let's also remove the filename argument (or rather, add a deprecation warning for now, remove it in v1.0)

NxNiki · 2024-03-21T00:56:57Z

I think this is perhaps an opportunity to formalize IO constructor arguments across all IO classes, before we get to the 1.0 release.

I think it will be confusing to have some IO classes that accept a list of things as the first argument, and others only a single thing. To solve the current issue, I support @zm711's suggestion of adding include_filenames, but then let's also remove the filename argument (or rather, add a deprecation warning for now, remove it in v1.0)

Yeah, that makes sense. Shall we ignore dirname and filename when using include_filenames or add files to filename/files in dirname? The later option seems logical, but the logic is kind of complex and the use case may be rare in practice.

apdavison · 2024-03-21T08:23:22Z

Shall we ignore dirname and filename when using include_filenames or add files to filename/files in dirname? The later option seems logical, but the logic is kind of complex and the use case may be rare in practice.

I don't use NeuralynxIO very much, so I will defer to those who do, but would suggest:

If include_filenames is not empty, then filename must be empty (otherwise raise exception), and
If dirname is not empty, then the contents of include_filenames should be interpreted as paths relative to dirname.

…o multiple_filenames

zm711

Couple comments about the current implementation.

neo/rawio/neuralynxrawio/neuralynxrawio.py

NxNiki · 2024-03-22T22:21:07Z

Hi, I am trying to run pytest, but it seems the environment cannot be correctly set up. Neither setup.py nor the pyproject.toml works for me.

zm711 · 2024-03-22T23:33:11Z

@NxNiki,

I routinely set up new environments building neo from source. What is the exact error you're getting. Note that we are still trying to work on making sure all tests work on Windows so testing may not be perfect on Windows machines (outside of core tests).

NxNiki · 2024-03-23T01:43:20Z

@NxNiki,

I routinely set up new environments building neo from source. What is the exact error you're getting. Note that we are still trying to work on making sure all tests work on Windows so testing may not be perfect on Windows machines (outside of core tests).

I am running the code on a Mac (M1 chip). The error when I use poetry to install the package is:

poetry install
[tool.poetry] section not found in /Users/XinNiuAdmin/Documents/python-neo/pyproject.toml.

I can use pip to install the packages with pip install . But running pytest has the following error:

======================================================================================================= test session starts =======================================================================================================
platform darwin -- Python 3.9.6, pytest-8.1.1, pluggy-1.4.0
rootdir: /Users/XinNiuAdmin/Documents/python-neo
configfile: pyproject.toml
collected 1389 items / 1 error                                                                                                                                                                                                    

============================================================================================================= ERRORS ==============================================================================================================
_________________________________________________________________________________________ ERROR collecting neo/test/iotest/test_tiffio.py _________________________________________________________________________________________
ImportError while importing test module '/Users/XinNiuAdmin/Documents/python-neo/neo/test/iotest/test_tiffio.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/Library/Developer/CommandLineTools/Library/Frameworks/Python3.framework/Versions/3.9/lib/python3.9/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
neo/test/iotest/test_tiffio.py:3: in <module>
    from PIL import Image
E   ModuleNotFoundError: No module named 'PIL'
===================================================================================================== short test summary info =====================================================================================================
ERROR neo/test/iotest/test_tiffio.py
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
======================================================================================================== 1 error in 1.01s =========================================================================================================

I guess this is also due to the incorrect pyproject.toml?

zm711 · 2024-03-23T11:46:04Z

@NxNiki,

If you want to run full tests you should do

pip install -e .[test]

You are missing a package. This is fine if you don't want to download a bunch of extra packages, but then rather than run pytest at the root you'll need to run individual tests instead. We don't have a poetry section in our code and I don't use poetry so I'm not sure how to install with poetry.

NxNiki · 2024-04-30T19:05:36Z

Hi, Sorry for the delay. I think this PR is ready to review. Let me know if there are additional comments.

zm711

Another round of comments @NxNiki.

It is very close. I think we would want to simplify the logic a bit in a few places where I indicated. You want to try those? and then I'll do another read. Just ping me again when you're ready.

neo/io/neuralynxio.py

neo/rawio/neuralynxrawio/neuralynxrawio.py

NxNiki · 2024-05-08T20:22:51Z

Hi @zm711, thanks for giving valuable suggestions. The PR is ready for another round of review.

zm711 · 2024-05-08T20:50:09Z

I'll give it another full read this weekend!

zm711

One last suggestion and one last discussion point, but then I think we are basically there.

zm711 · 2024-05-10T13:43:03Z

neo/rawio/neuralynxrawio/neuralynxrawio.py

+        elif isinstance(include_filenames, str):
+            include_filenames = [include_filenames]


This is the one I'm still a little torn on. I would prevent to force include_filenames to be a list, but I think it is okay if we accept a string in the case of only wanting one file.

Under the hood, include_filenames has to be a list, or the iteration at line 220 would loop over each string character.

Sorry I wasn't clear. I was saying why should we accept a string at all (so delete line 165 and 166). Since our new argument is filenames shouldn't we just always make the users give a list (or any iterable)? Why should we accept a single file rather than a list of a single file?

I asked on Chatgpt and it seems quite common to allow for flexible input types in this case. I am open to both ways though.

Here is the answer from chatgpt:

In Python, when designing an I/O interface to read a list of files, it's generally user-friendly to allow for flexible input types. Here are the considerations for both approaches you mentioned:

Allowing a string or list as input:

Pros: More convenient for users who often deal with a single file. It reduces the overhead of wrapping a filename in a list, making the function easier to use in scripts or interactive sessions where only one file is processed.
Cons: The function implementation needs to handle different types of input, checking whether the input is a string or a list. This can make the function slightly more complex.
Strictly requiring a list of strings:

Pros: Consistency in input type can simplify the function's logic, as it always expects a list. This can make the code simpler and potentially less error-prone when dealing with multiple files.
Cons: Less convenient for cases where only a single file is involved, as the user must wrap the filename in a list even if it's just one.
Given these considerations, a common practice is to design the interface to accept both strings and lists. This approach enhances usability without significantly complicating the implementation.

@apdavison did you have opinions on this? I prefer to be more restrictive, but I know other parts of Neo's API is relatively permissive. I think you prefer more flexibility too @samuelgarcia, so I'm fine with being outvoted here.

I think it's a tough call, but I can't see that accepting a string will cause major problems, so I'm happy to go with the current interface.

neo/rawio/neuralynxrawio/neuralynxrawio.py

Co-authored-by: Zach McKenzie <[email protected]>

zm711 · 2024-05-21T12:07:19Z

@NxNiki, we have a conference that a bunch of us are attending next week, but we will try to get final review done the week after that!

NxNiki · 2024-05-21T21:33:34Z

@NxNiki, we have a conference that a bunch of us are attending next week, but we will try to get final review done the week after that!

Thanks for the update. I hope you have a nice trip!

zm711 · 2024-06-06T16:18:35Z

@apdavison did you want to check out the argument reorganization on this one before we have our meeting?

zm711 · 2024-07-02T11:59:13Z

@apdavison, just wanted to ping you again for giving this one a read!

NxNiki · 2024-07-23T21:23:14Z

@apdavison, just wanted to ping you again for giving this one a read!

Hi, any updates? Let me know if there are additional comments or concerns.

apdavison · 2024-07-24T08:00:14Z

Many thanks @NxNiki for all your work on this, and sorry for the delayed response from my side.

NxNiki · 2024-07-24T20:40:02Z

Many thanks @NxNiki for all your work on this, and sorry for the delayed response from my side.

NP! This is a great learning opportunity for me. Thanks for the discussions and comments!

samuelgarcia · 2024-07-25T16:16:22Z

neo/rawio/neuralynxrawio/neuralynxrawio.py

@@ -183,33 +213,24 @@ def _parse_header(self):
        event_annotations = []

        if self.rawmode == "one-dir":
-            filenames = sorted(os.listdir(self.dirname))
-            dirname = self.dirname
+            filenames = os.listdir(self.dirname)


I am realising only now that this little has an bi big impact because the file are unsorted and so channel are not ordered

See this SpikeInterface/spikeinterface#3248

You think this is Anthony's issue? This was just merged yesterday, so I guess we could confirm that the issue showed up, but makes sense that could be the problem. You plan to just open the PR?

Oh, sorry for missing this issue. Do we also want to sort include_filenames? One edge case would be the user using os.listdir to collect the files and pass it to include_filenames.

No wory. @Antho2422 did a quick fix.
For include_filenames, I think we do not need the sorter() because it makes that the user can providde a list in the desired order. no ?

NxNiki added 2 commits March 19, 2024 11:06

support List[str] for filename

0742c75

update _source_name

fb84fe7

zm711 changed the title ~~support List[str] for filename~~ NeuralynxIO: support List[str] for filename parameter Mar 20, 2024

zm711 requested a review from samuelgarcia March 20, 2024 12:25

NxNiki added 5 commits March 21, 2024 13:06

temp master

38d60b3

add include_files to IO constructor

0f37314

fix one-file rawmode

d1462c7

fix base io api

37e9678

Merge branch 'multiple_filenames' of github.com:NxNiki/python-neo int…

e50a1bb

…o multiple_filenames

zm711 reviewed Mar 22, 2024

View reviewed changes

neo/rawio/neuralynxrawio/neuralynxrawio.py Outdated Show resolved Hide resolved

neo/rawio/neuralynxrawio/neuralynxrawio.py Outdated Show resolved Hide resolved

NxNiki added 3 commits March 22, 2024 10:54

fix _source_name

e229076

remove check number of dirs in include_filename

1185079

do not change dirname to empty in multiple-files mode

cfa1045

NxNiki added 2 commits March 22, 2024 19:10

fix bug when include_filename is None (not convert it to [None])

0ff957a

fix bug for hidden files and directory in <dirname>

51b7912

NxNiki added 2 commits March 27, 2024 18:12

fix bug in checking exclude_filename

f23efe5

fix bug in exclude_filename

e3cef2c

fix warnings

62a5512

NxNiki requested review from zm711 and alejoe91 April 30, 2024 19:05

zm711 reviewed Apr 30, 2024

View reviewed changes

NxNiki added 4 commits May 6, 2024 10:22

check if filenames is None and change them to list

624b3c9

change exclude_filenames from set to list

9539548

bug fix in single include_filename

b92568f

bug fix on exclude filenames

28af705

comment test

c554ef3

zm711 reviewed May 10, 2024

View reviewed changes

Update neo/rawio/neuralynxrawio/neuralynxrawio.py

3621ecf

Co-authored-by: Zach McKenzie <[email protected]>

zm711 mentioned this pull request Jun 28, 2024

[Bug]: NeuralynxRecordingInterface fails on Neuralynx data with multiple streams catalystneuro/neuroconv#927

Open

2 tasks

zm711 requested a review from apdavison July 23, 2024 21:31

apdavison merged commit ad38785 into NeuralEnsemble:master Jul 24, 2024
2 checks passed

zm711 mentioned this pull request Jul 24, 2024

Direct assignment of multiple '.ncs' files in neo.io.NeuralynxIO(dirname=session_data) #1439

Closed

samuelgarcia reviewed Jul 25, 2024

View reviewed changes

samuelgarcia mentioned this pull request Jul 25, 2024

Channel ids are mixed up when loading recording SpikeInterface/spikeinterface#3248

Closed

Antho2422 mentioned this pull request Jul 25, 2024

Added a sorted to the listdir of the files in neuralynx folder to avo… #1512

Merged

zm711 modified the milestones: 0.14.0, 13.2 Jul 26, 2024

		elif isinstance(include_filenames, str):
		include_filenames = [include_filenames]

NeuralynxIO: support List[str] for filename parameter #1440

NeuralynxIO: support List[str] for filename parameter #1440

Conversation

NxNiki commented Mar 19, 2024 • edited Loading

zm711 commented Mar 20, 2024 • edited Loading

samuelgarcia commented Mar 20, 2024

zm711 commented Mar 20, 2024

NxNiki commented Mar 20, 2024 • edited Loading

zm711 commented Mar 20, 2024

NxNiki commented Mar 20, 2024

apdavison commented Mar 20, 2024

NxNiki commented Mar 21, 2024 • edited Loading

apdavison commented Mar 21, 2024

zm711 left a comment

Choose a reason for hiding this comment

NxNiki commented Mar 22, 2024

zm711 commented Mar 22, 2024

NxNiki commented Mar 23, 2024

zm711 commented Mar 23, 2024

NxNiki commented Apr 30, 2024

zm711 left a comment

Choose a reason for hiding this comment

NxNiki commented May 8, 2024

zm711 commented May 8, 2024

zm711 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zm711 commented May 21, 2024

NxNiki commented May 21, 2024

zm711 commented Jun 6, 2024

zm711 commented Jul 2, 2024

NxNiki commented Jul 23, 2024

apdavison commented Jul 24, 2024

NxNiki commented Jul 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NxNiki commented Mar 19, 2024 •

edited

Loading

zm711 commented Mar 20, 2024 •

edited

Loading

NxNiki commented Mar 20, 2024 •

edited

Loading

NxNiki commented Mar 21, 2024 •

edited

Loading