cbrnr / mnelab

MNELAB – a GUI for MNE
BSD 3-Clause "New" or "Revised" License
239 stars 69 forks source link

JOSS review - vahid-sb #359

Closed vahid-sb closed 2 years ago

vahid-sb commented 2 years ago

Hi guys As part of the review of your JOSS submissions, I have been testing MNELAB for the past few days. And decided to open this issue to mention anything that need fixing. I will add more if I notice more issues. But before that, I should congratulate you all on this repository. Obviously, there is still many features that can be added, but as it is, it is already a very impressive and useful library. I tested MNELAB under both Linux (Ubuntu 21) and Windows 10. In both cases, I created a conda environment using conda create -n mnelab python=3.9 and after activating the environment, installed mnelab using: pip install mnelab[full] In both cases, I used output data from Branvision Acticap-64 and also from Wearable Sensing DSI-7 recorders.

  1. The first issue is that in Windows, I can plot the time-series data, but when trying to plot PSD, I get the following error:
Traceback (most recent call last):
  File "C:\Users\DNI-2\miniconda3\envs\mnelab2\lib\site-packages\mnelab\mainwindow.py", line 791, in plot_psd
    fig = self.model.current["data"].plot_psd(show=False, **kwds)
  File "<decorator-gen-220>", line 12, in plot_psd
  File "C:\Users\DNI-2\miniconda3\envs\mnelab2\lib\site-packages\mne\io\base.py", line 1548, in plot_psd
    return plot_raw_psd(self, fmin=fmin, fmax=fmax, tmin=tmin, tmax=tmax,
  File "<decorator-gen-187>", line 12, in plot_raw_psd
  File "C:\Users\DNI-2\miniconda3\envs\mnelab2\lib\site-packages\mne\viz\raw.py", line 438, in plot_raw_psd
    fig = _psd_figure(
  File "C:\Users\DNI-2\miniconda3\envs\mnelab2\lib\site-packages\mne\viz\_mpl_figure.py", line 2172, in _psd_figure
    psd, freqs = psd_func(inst, tmin=tmin, tmax=tmax, picks=picks,
  File "<decorator-gen-138>", line 12, in psd_welch
  File "C:\Users\DNI-2\miniconda3\envs\mnelab2\lib\site-packages\mne\time_frequency\psd.py", line 287, in psd_welch
    return psd_array_welch(data, sfreq, fmin=fmin, fmax=fmax, n_fft=n_fft,
  File "<decorator-gen-137>", line 12, in psd_array_welch
  File "C:\Users\DNI-2\miniconda3\envs\mnelab2\lib\site-packages\mne\time_frequency\psd.py", line 202, in psd_array_welch
    f_spect = parallel(my_spect_func(d, func=func, freq_sl=freq_sl,
  File "C:\Users\DNI-2\miniconda3\envs\mnelab2\lib\site-packages\joblib\parallel.py", line 1001, in __call__
    self._print("Using backend %s with %d concurrent workers.",
  File "C:\Users\DNI-2\miniconda3\envs\mnelab2\lib\site-packages\joblib\parallel.py", line 871, in _print
    writer = sys.stderr.write
AttributeError: 'NoneType' object has no attribute 'write'

While in Linux I could plot PSD with no error.

vahid-sb commented 2 years ago
  1. When opening a file which is not in a standard EEG format, I get the following error. To make the error message more informative for the user, please add an exception handling to the below mentioned line with a proper message that states file type not supported, rather than "TypeError: cannot unpack non-iterable NoneType object".
Traceback (most recent call last):
  File "C:\Users\DNI-2\miniconda3\envs\mnelab2\lib\site-packages\mnelab\mainwindow.py", line 578, in open_data
    self.model.load(fname)
  File "C:\Users\DNI-2\miniconda3\envs\mnelab2\lib\site-packages\mnelab\model.py", line 36, in wrapper
    f(*args, **kwargs)
  File "C:\Users\DNI-2\miniconda3\envs\mnelab2\lib\site-packages\mnelab\model.py", line 110, in load
    data = read_raw(fname, *args, **kwargs, preload=True)
  File "C:\Users\DNI-2\miniconda3\envs\mnelab2\lib\site-packages\mnelab\io\readers.py", line 79, in read_raw
    _, ext = split_name_ext(fname)
TypeError: cannot unpack non-iterable NoneType object
cbrnr commented 2 years ago

Regarding the first issue, you have hit an edge case which occurs only on Windows using the launcher mnelab and joblib installed. Among other things, this is because the mnelab launcher, which is auto-generated by pip, redirects stdout and stderr, which leads to this error (this had more severe consequences that have been fixed, but if you are interested this is probably the most relevant issue).

There are three potential solutions:

  1. Start MNELAB with python -m mnelab instead of mnelab.
  2. Uninstall joblib, then everything works even with the mnelab launcher.
  3. Update MNE-Python to the latest dev version (pip install -U git+https://github.com/mne-tools/mne-python); it seems like the recent changes related to computing PSDs also fixed this issue, so everything works even with the mnelab launcher.

MNE 1.2 will be released very soon, and then solution 3 will be automatically implemented. Therefore, could you please check if those three options (and especially the last one) work for you? If so, I think it's OK to wait until MNE 1.2 has been released.

cbrnr commented 2 years ago

I've fixed the second issue in #360.

cbrnr commented 2 years ago

@vahid-sb are you happy with the changes? If not, let me know what else needs to be fixed.

vahid-sb commented 2 years ago

@cbrnr sorry for the delay in responding. I just found time to check both fixes and both issues are now resolved. Thanks for that. I will perform some more checks and tests and if I face no other issues, will update the review form accordingly.

vahid-sb commented 2 years ago

I did more tests and looked into the code, and no did not encounter any others issues. Hence, I close this issue. Thanks for resolving the previous ones.