NeuralynxIO: support List[str] for filename parameter

[NxNiki]

Add a feature to allow multiple files as filename.

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

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

[zm711]

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]

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

[zm711]

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]

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]

@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]

@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]

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]

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]

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:

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

[NxNiki]

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]

@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]

@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]

@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.

[zm711]

@NxNiki,

I would probably avoid fixing typos in this PR for other files since the focus on this PR should be getting your changes to neuralynx to pass the testing. We appreciate typo fixes, but those are better in a dedicated PR (that way we can follow a flow of one PR does one thing).

[NxNiki]

@NxNiki,

I would probably avoid fixing typos in this PR for other files since the focus on this PR should be getting your changes to neuralynx to pass the testing. We appreciate typo fixes, but those are better in a dedicated PR (that way we can follow a flow of one PR does one thing).

Yeah, that makes sense. Will revert the fix topo change.

[zm711]

@samuelgarcia I think this is ready for you to decide on.

[zm711]

@NxNiki,

So the Neo maintenance team met today and discussed that we want to move ahead with regularizing the constructors for IOs. That would mean for this io we would want just to accept a dirname. We would want to deprecate the use of filename and so if someone wanted a single file they would need to use dirname='my_data/folder', include_filenames=['one_file']. We would also want to deprecate the include_filename and exclude_filename in favor of include_filenames and exclude_filenames. Would you be willing to incorporate those changes to this PR.

[NxNiki]

@NxNiki,

So the Neo maintenance team met today and discussed that we want to move ahead with regularizing the constructors for IOs. That would mean for this io we would want just to accept a dirname. We would want to deprecate the use of filename and so if someone wanted a single file they would need to use dirname='my_data/folder', include_filenames=['one_file']. We would also want to deprecate the include_filename and exclude_filename in favor of include_filenames and exclude_filenames. Would you be willing to incorporate those changes to this PR.

Yeah, this makes sense. Will keep working on this PR to make the change!

[zm711]

Thanks we really appreciate it!

[NxNiki]

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

[NxNiki]

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

[zm711]

I'll give it another full read this weekend!

[zm711]

@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]

@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]

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

[zm711]

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

[NxNiki]

@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]

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

[NxNiki]

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!