neuromodulation / py_neuromodulation

Real-time analysis of intracranial neurophysiology recordings.
https://neuromodulation.github.io/py_neuromodulation/
MIT License
41 stars 9 forks source link

Fix type hints project-wide #303

Closed toni-neurosc closed 2 months ago

toni-neurosc commented 2 months ago

Since Pydantic will be used to validate analysis settings, and the contents of settings will be propagated to the different submodules of PyNeuro, type hints should be consistently provided, and correct, all accross the project. MyPy and PyRight are throwing a lot of static analysis errors at the time, which is unclear if they would affect Pydantic integration or not, but I think it's better to fix them, also in case users of the module are using static analysis tools themselves.

mypy_report.txt pyright_report.txt

timonmerk commented 2 months ago

Thanks for running those tests @toni-neurosc. It would be great to take this as an opportunity to refactor some parts of the current codebase. I had a look at the files, and found that we can for now ignore:

For both I need to check what is currently necessary, and how dependencies can be reduced.

Furthermore, I would even leave out certain modules and delete them in the dev branch for refactoring that would clean up the existing codebase:

Apart of that the main point would be to improve the validation with pydantic as an object instead of an dictionary.

toni-neurosc commented 2 months ago

Hi @timonmerk, so I've been perusing the codebase while reviewing the error reports that I appended in the first comment. I fixed a good number of them in PR #305 and also spotted some less trivially fixable errors and some things that I think need reviewing. I will be opening some issues in the following days regarding that. I did not touch the files you listed very much, only very minor things, I also excluded them from MyPy checks.

The errors and warnings that remain, some are perhaps bugs (will open issues), some are typing errors that are not so easy to fix but I think Pydantic will help fixing those by using validators: https://docs.pydantic.dev/latest/concepts/validators

While I was doing that I was also ruminating on how to implement the settings class in a way that checks all the requirements we talked about in the meeting and I think I came up with a good solution, so you'll be hearing about that soon.

I'll keep the issue open until you review the PR, may take you a while cause it's a lot of scattered changes.

toni-neurosc commented 2 months ago

Closed with the merge of #305