Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unused dependencies - review examples and tutorials #188

Merged
merged 20 commits into from
Jun 2, 2021

Conversation

sylvchev
Copy link
Member

@sylvchev sylvchev commented May 7, 2021

closes #180

This issue is connected with the problem in documentation generation #173 as wfdb silently fails to download the file and when the example code extracts MNE epochs from downloaded files, they are empty.

I contacted Dr. Spiridon Nikolopoulos, that posted the MAMEM dataset on figshare. Unfortunately, these are only big rar/zip files that contain .mat files for each session/subject. I worked on the figshare to upload the separate files, it is okay for MAMEM 2 and 3.

I added all the code to download and open mat files instead of wfdb format. The data and labels obtained with mat files are similar to those obtained with wfdb.

This PR also introduces some new code to download dataset without using MNE internal fetch function. As they told us, it would be deprecated. They recommend to use pooch. I added helper function in download.py to get FigShare file using pooch.

There is still a problem with MAMEM 1 dataset, because of an internal error of FigShare. Maybe it is possible to merge this code without waiting for full support of the MAMEM 1.

Sylvain Chevallier added 17 commits May 4, 2021 02:14
Signed-off-by: Sylvain Chevallier <sylvain.chevallier@uvsq.fr>
Signed-off-by: Sylvain Chevallier <sylvain.chevallier@uvsq.fr>
Signed-off-by: Sylvain Chevallier <sylvain.chevallier@uvsq.fr>
Signed-off-by: Sylvain Chevallier <sylvain.chevallier@uvsq.fr>
Signed-off-by: Sylvain Chevallier <sylvain.chevallier@uvsq.fr>
Signed-off-by: Sylvain Chevallier <sylvain.chevallier@uvsq.fr>
Signed-off-by: Sylvain Chevallier <sylvain.chevallier@uvsq.fr>
Signed-off-by: Sylvain Chevallier <sylvain.chevallier@uvsq.fr>
Signed-off-by: Sylvain Chevallier <sylvain.chevallier@uvsq.fr>
Signed-off-by: Sylvain Chevallier <sylvain.chevallier@uvsq.fr>
Signed-off-by: Sylvain Chevallier <sylvain.chevallier@uvsq.fr>
Signed-off-by: Sylvain Chevallier <sylvain.chevallier@uvsq.fr>
Signed-off-by: Sylvain Chevallier <sylvain.chevallier@uvsq.fr>
Signed-off-by: Sylvain Chevallier <sylvain.chevallier@uvsq.fr>
Signed-off-by: Sylvain Chevallier <sylvain.chevallier@uvsq.fr>
Signed-off-by: Sylvain Chevallier <sylvain.chevallier@uvsq.fr>
@sylvchev sylvchev self-assigned this May 17, 2021
@sylvchev sylvchev added dataset Supporting new dataset enhancement labels May 17, 2021
@sylvchev sylvchev added this to the 0.4.0 milestone May 17, 2021
@sylvchev
Copy link
Member Author

The figshare issue is solved. I could remove the wfdb dependency. Also, download.data_path that relies on MNE _fetch_file is now deprecated in favor of data_dl (that relies on pooch.retrieve). The data_dl allows to support the download through http, https, ftp and sftp. This allows to solve the FTP issue #77 for Cho2017 and Lee2019. Also, I updated the Wang2016 to use FTP, thus downloading directly mat file instead of rar archive. This allows to drop the pyunpack and patool dependencies. While replacing the data_path by data_dl, I corrected a regression error on Weibo2014 dataset (the ch_names was incomplete causing error).

I added the request dependency for figshare download support, it is important as MNE is will not provide support for fetching datasets.

In order to ease the documentation building, I reduced the number of subjects use in the examples and tutorials. Also, I picked smaller datasets and tried to reuse the same datasets across examples to limit the number data to download. I corrected the Result class, because session and subject field where uncorrectly typed as byte instead of string, resulting in wrong displayed information in example plots. I corrected the default storage of hdf5 file for Result class, using an MNE compliant path and documenting it in the tutorial. The tutorials file encoding was incorrect (part DOS and Linux end-of-line) and it is homogenous now.

Closes #77

Sylvain Chevallier added 2 commits May 18, 2021 01:28
Signed-off-by: Sylvain Chevallier <sylvain.chevallier@uvsq.fr>
Signed-off-by: Sylvain Chevallier <sylvain.chevallier@uvsq.fr>
@sylvchev sylvchev linked an issue May 17, 2021 that may be closed by this pull request
@sylvchev
Copy link
Member Author

This code is stable and is ready to merge. I tested it against all examples and tutorials.

@jsosulski @v-goncharenko or @ErikBjare
Do you have the time to double check this PR? We could move to fix #174 afterwards.

@jsosulski
Copy link
Collaborator

I can take a look on the weekend!

@sylvchev sylvchev changed the title Remove wfdb dependency Remove unused dependencies - review examples and tutorials May 19, 2021
@sylvchev sylvchev requested a review from jsosulski May 19, 2021 07:21
Copy link
Collaborator

@jsosulski jsosulski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me aside from some minor comments. :) thanks

moabb/datasets/download.py Outdated Show resolved Hide resolved
moabb/datasets/download.py Show resolved Hide resolved
moabb/datasets/download.py Show resolved Hide resolved
moabb/datasets/gigadb.py Show resolved Hide resolved
if (current - previous) > thres_split:
sampleB = samples[i - 1]
freqs.append(s // c)
if (sampleB - sampleA) > 382:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is mamem specific, but why is the diff compared to 382?

moabb/datasets/ssvep_mamem.py Show resolved Hide resolved
Signed-off-by: Sylvain Chevallier <sylvain.chevallier@uvsq.fr>
@sylvchev
Copy link
Member Author

sylvchev commented Jun 1, 2021

Ok, I'll merge this one!

@sylvchev sylvchev merged commit 3d42d79 into master Jun 2, 2021
@sylvchev sylvchev deleted the hotfix/remove-wfdb branch September 3, 2021 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Removing wfdb dependency Cho2017 download unit test
2 participants