LMBooth / pybci

Create real-time BCI's with the LSL, PyTorch, SKLearn and TensorFlow packages.
https://pybci.readthedocs.io/en/latest/
MIT License
22 stars 4 forks source link

some import errors such as NameError #18

Closed anilbey closed 1 year ago

anilbey commented 1 year ago

I've noticed a potential oversight in PseudoDevice.py where certain lines are likely to raise NameErrors due to the absence of the multiprocessing import. Specifically, these lines are:

1) https://github.com/LMBooth/pybci/blob/f8fcdbaacded50138a0e27a88f36683e42b22c4e/pybci/Utils/PseudoDevice.py#L119 2) https://github.com/LMBooth/pybci/blob/f8fcdbaacded50138a0e27a88f36683e42b22c4e/pybci/Utils/PseudoDevice.py#L125

For clarity, here's a snippet illustrating the error:

>>> from multiprocessing import Queue
>>> multiprocessing.Queue
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
NameError: name 'multiprocessing' is not defined
>>> 

This brings up three key concerns:

1) NameError due to the missing import statements in two locations. 2) The tests currently do not capture this NameError, suggesting potential gaps in our test coverage. 3) Additionally, there are several imports present in the codebase that are not in use.

To mitigate the first and third issues, utilizing code linters, such as ruff, could be beneficial:

pip install ruff
ruff .
pybci/ThreadClasses/OptimisedDataReceiverThread.py:1:19: F401 [*] `time` imported but unused
pybci/ThreadClasses/OptimisedDataReceiverThread.py:2:25: F401 [*] `collections.deque` imported but unused
pybci/ThreadClasses/OptimisedDataReceiverThread.py:3:8: F401 [*] `itertools` imported but unused
pybci/ThreadClasses/OptimisedDataReceiverThread.py:5:20: F401 [*] `bisect.bisect_left` imported but unused
pybci/ThreadClasses/OptimisedDataReceiverThread.py:128:35: E712 Comparison to `False` should be `cond is False` or `if not cond:`
pybci/Utils/Classifier.py:4:8: F401 [*] `tensorflow` imported but unused

Incorporating a linter into the CI pipeline will help prevent such oversights in future commits.

For the second concern, generating a code coverage report would provide insights into the areas currently untested.

This issue is a part of https://github.com/openjournals/joss-reviews/issues/5706.

LMBooth commented 1 year ago

Hi @anilbey , thanks for recommending the linter and coverage report! I've ran ruff and cleaned up a lot of the unnecessary imports and added it in to the appveyor process to help identify future issues like you've recommended.

For your other recomendation, I ran some coverage checks locally which is returning results but seem to be having a bit of trouble getting anything back using Codecov in a workflow, hopefuly will crack it when i finish work tomorrow evening. I've tried extending some of the tests to also work with the cli commands too but been having trouble getting them to return properly so may exclude them from the coverage for now.

LMBooth commented 1 year ago

For an update, managed to get the codecov report working and added the shield to the readme.md in the root of the repository. Currently trying to expand the tests to cover more of the codebase. Thanks again @anilbey , i'll leave the issue open for now until i manage to finish expanding my test coverage.

LMBooth commented 1 year ago

Update: target coverage >70%