Fix type hints, clean imports, and other errors reported by static analysis

[toni-neurosc]

So in regards to #303 I passed 3 different static analyzers over the codebase (Ruff, PyRight and Mypy) and fixed a bunch of stuff of varying grades of relevance. The goals were:

• To overall improve the consistency of the code and adherence to latest standards
• To make sure default values for arguments are handled correctly (e.g. in the case of None defaults)
• To clean up error reports of trivial warnings and flags so that potential serious errors are easier to spot in future static analyses
• To perhaps improve startup time by cleaning unused imports

In terms of results, they were:

• For Mypy, going from 272 errors to 24 errors. These 24 errors are now things that actually require fixing, see report file below generated by this command: mypy --follow-imports skip --exclude nm_cohortwrapper\.py --exclude nm_across_patient_decoding\.py --exclude nm_eval_timing\.py --exclude nm_nolds\.py --exclude FieldTrip\.py --exclude nm_RMAP\.py --exclude nm_decode\.py py_neuromodulation\

mypy_report_23_04_24.txt

• For PyRight, from [287 errors, 2 warnings] to [168 errors, 0 warnings] pyright_types.txt

A non-exhaustive but quite complete list of the changes done are:

• Remove unused imports
• Fix incorrect type typehints (e.g. bool = None, or np.array instead of np.ndarray)
• Change Optional and Union typehints to modern style i.e. list[str|None] or int | None
• Add comments to potential bugs, check commit (e.g. duplicated "s" argument to axis.scatter)
• Fix empty error raises with corresponding errors
• Commented out variables that were created but never used, feel free to remove
• Commented out class-level variables that were intended to be instance-level variables (i.e. variables defined at top-indentation level within a class and without self. prefix)
• And a bit of perhaps a controversial one: Favor empty lists and empty strings as default values for arguments as opposed to None values. Reasons are several:
  • static analysis complains a lot about None obviously not having any methods or being passable to functions.
  • it generates a lot of type analysis cascades of errors (e.g. when the input to a function can be str | None but that input is passed to a function that only accepts str and doesn't even handle the None case)
  • If i'm not mistaken, passing empty lists, arrays or strings is never an expected input for the functions where I changed this, so handling the empty case instead of the None case should not generate any unexpected behaviours

I would understand if the last change is not accepted, for me it seems more intuitive to check for empty inputs or sentinel values rather than Python NoneType, but it's a matter of preference at the end of the day.

[timonmerk]

@toni-neurosc thanks again for those changes and mentioning about the mypy errors. I tried to solve them and also fixed a couple of them.

It raised an issue with the preprocessing classes. Currently there is no good method to call the tests. I thought about it for some time now but either the tests are called locally within the init method, which is a good way to ensure also testing before instantiation. But on the other hand it would be nice to call it also externally from nm_run_analysis when the preprocessors are instantiated..

Plus there is not enough testing implemented for all preprocessors that sometimes the functions would be empty. So I decided to remove the testing method currently. Not sure, we could also have a similar implementation with the features that they are called on the class with the same params but then also some tests brake s.t. the instantiation without nm_run_analysis should also break with wrong params, meaning that I want to call the tests from the init method. Quite a design question.. but I would say let's leave it for now with this method even if it's not super elegant.

[toni-neurosc]

Hi @timonmerk, don't worry about the test_settings methods, that will be lumped together with the settings class, the FeatureSettings class/protocol, and the preprocessor settings class/protocol. In case a preprocessor does not have settings to test, i don't think its a problem to leave the function empty, but they way you left it now looks good to me for the time being.

Also, I did some minor changes to the testing scripts, just removing unused imports and fix a couple types that didn't match. I think it's not affecting function but since you know better than me the purpose of each test is best you take a look at it.

Also, I realize I messed up the type and default value of the "clip" parameter for _normalization_and_clip, now it's fine, it defaults to 0 which when arrives to the normalization function will test False and be replaced with the default 3.

[toni-neurosc]

Ok, so I added formatting with ruff with would close #309 . We may also add a pre-commit hook at some point in the future https://github.com/astral-sh/ruff-pre-commit

[toni-neurosc]

I feel like there are some changes that I have not explained and I should mention here for clarity:

• Path manipulation was being done with os and pathlib interchangeably. Since pathlib is the more "modern" Python approach and it's being actively developed I decided to change all the instances of os to the pathlib equivalents, and also simplified some string manipulation logic in nm_IO.py.
  • The only exception where I could not get rid of os was one instance of os.walk(), that in Python 3.10 does not have an equivalent in pathlib. In Python 3.12 Path.walk() was introduced so when we bump the Python version requirements to 3.12 or higher we can uncomment that line.
• I removed some dependencies in pyproject.toml that were not being used in the project. They were:
  • pybids
  • pynput
  • pip
  • wget -> This one because it's not maintained and urllib.requests can replace it easily
• I added statsmodels as a dependency, it was being used but not listed, probably it was itself a dependency of one of the packages I removed.
• I realized I removed comparisons to True but not to false so I have replaced instances of if ____ is not False: with if not ____:

[timonmerk]

@toni-neurosc Thanks a lot for all those changes. It improved the readability of the code quite a lot and will hopefully also make the integration with pydantic easier. Unfortunately it is now at a stage where it's very difficult to review the changes, and it's many files that have been changed. So I propose that you make the merge into main yourself. Unless there are specific files that you think I need to check?

I tried to merge the pyproject.toml but there was a syntax error. Additionally, I merged the GitHub workflow PR already into main, so it may require checking that those changes are compatible.

I think those changes do not brake backwards compatibility, since it doesn't break the API. But once the tests run through, now also for Windows, feel free to merge into main.

[toni-neurosc]

Thanks a lot for trusting me with the merge @timonmerk ! I have been working today on this for quite a while and I think it's safe to merge, I did indeed introduce a couple bugs that I had to solve, but I think they were the only ones and since they were related to typing and input validation anyway, if there are other bugs that I missed they will most likely be caught soon enough in my next PRs. So I'm closing this one up, and I hope I never have to submit such a massive PR again xD