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

Add Thielen2015 c-VEP dataset #557

Merged
merged 15 commits into from Apr 11, 2024
Merged

Conversation

thijor
Copy link
Contributor

@thijor thijor commented Apr 8, 2024

This pull request adds the Thielen et al. 2015 c-VEP dataset. Additionally, it fixes issue #555 regarding the Thielen et al. 2021 dataset, and issue #554 that fixes a bug and makes datasets in the c-VEP paradigm searchable.

@PierreGtch PierreGtch self-requested a review April 8, 2024 14:42
@PierreGtch
Copy link
Collaborator

Fixes #554 Fixes #555

Copy link
Collaborator

@PierreGtch PierreGtch left a comment

Choose a reason for hiding this comment

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

@thijor Thielen2015 and Thielen2021 share multiple methods copy/pasted methods. I think it would make sense to define them both in the same thielen.py file with a common parent class to facilitate maintenance in the future. Lee2019 for example does this

docs/source/dataset_summary.rst Outdated Show resolved Hide resolved
docs/source/whats_new.rst Outdated Show resolved Hide resolved
docs/source/whats_new.rst Outdated Show resolved Hide resolved
docs/source/whats_new.rst Outdated Show resolved Hide resolved
moabb/datasets/thielen2021.py Show resolved Hide resolved
@thijor
Copy link
Contributor Author

thijor commented Apr 10, 2024

@thijor Thielen2015 and Thielen2021 share multiple methods copy/pasted methods. I think it would make sense to define them both in the same thielen.py file with a common parent class to facilitate maintenance in the future. Lee2019 for example does this

You are right, Pierre. However, also the castillos2023 dataset uses these functions, as these are the ones that embed epoch and trial information in the datasets as separate stim channels. So a general thielen.py is not handy. Would be more like utils. Where would you want to put it then?

@PierreGtch
Copy link
Collaborator

@thijor Thielen2015 and Thielen2021 share multiple methods copy/pasted methods. I think it would make sense to define them both in the same thielen.py file with a common parent class to facilitate maintenance in the future. Lee2019 for example does this

You are right, Pierre. However, also the castillos2023 dataset uses these functions, as these are the ones that embed epoch and trial information in the datasets as separate stim channels. So a general thielen.py is not handy. Would be more like utils. Where would you want to put it then?

Then I think these functions would fit in moabb/datasets/utils.py

docs/source/whats_new.rst Outdated Show resolved Hide resolved
@PierreGtch PierreGtch enabled auto-merge (squash) April 11, 2024 11:31
@PierreGtch PierreGtch merged commit 097b057 into NeuroTechX:develop Apr 11, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants