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

[Dataset] Brain Invaders dataset with VR/PC display #358

Merged
merged 46 commits into from
Apr 20, 2023

Conversation

gcattan
Copy link
Contributor

@gcattan gcattan commented Apr 7, 2023

@sylvchev This PR is a follow-up on the work you did already with bi2014/bi2015.
It is a proposal to integrate the Brain Invaders with VR dataset (https://github.com/plcrodrigues/py.VR.EEG.2018-GIPSA) in MOABB.

@plcrodrigues FYI

moabb/datasets/braininvaders.py Outdated Show resolved Hide resolved
moabb/datasets/braininvaders.py Outdated Show resolved Hide resolved
moabb/datasets/braininvaders.py Outdated Show resolved Hide resolved
moabb/datasets/braininvaders.py Show resolved Hide resolved
moabb/datasets/braininvaders.py Outdated Show resolved Hide resolved
@bruAristimunha
Copy link
Collaborator

Hi @gcattan!

So nice, a new dataset! Very happy with your addition!

In addition to the comments, some small points:

  1. Can you try to create some tests for the dataset?
  2. Can you update the what's new file?
  3. Can you update the dataset summary?

Copy link
Collaborator

@bruAristimunha bruAristimunha left a comment

Choose a reason for hiding this comment

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

Fixing the test and these new suggestions, LGTM!

docs/source/whats_new.rst Show resolved Hide resolved
docs/source/dataset_summary.rst Show resolved Hide resolved
moabb/datasets/braininvaders.py Show resolved Hide resolved
moabb/datasets/braininvaders.py Outdated Show resolved Hide resolved
moabb/datasets/braininvaders.py Show resolved Hide resolved
@bruAristimunha
Copy link
Collaborator

Everything is fine for me! @sylvchev, can you give the final check and apply the merge?

docs/source/dataset_summary.rst Show resolved Hide resolved
docs/source/dataset_summary.rst Show resolved Hide resolved
Copy link
Member

@sylvchev sylvchev left a comment

Choose a reason for hiding this comment

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

Thanks @gcattan for this nice PR. I made some suggestions, could you make the change ? We could merge your PR afterwards.
The get_block_repetition is very nice, it could be very interesting to generalized this approach to other P300 datasets, where it could be applied.

moabb/datasets/braininvaders.py Outdated Show resolved Hide resolved
moabb/datasets/braininvaders.py Outdated Show resolved Hide resolved
moabb/datasets/braininvaders.py Outdated Show resolved Hide resolved
moabb/datasets/braininvaders.py Outdated Show resolved Hide resolved
moabb/datasets/braininvaders.py Outdated Show resolved Hide resolved
moabb/datasets/braininvaders.py Outdated Show resolved Hide resolved
moabb/datasets/braininvaders.py Outdated Show resolved Hide resolved
@gcattan
Copy link
Contributor Author

gcattan commented Apr 19, 2023

Thanks for the review @sylvchev. Yes, Pedro had a nice idea with the get_block_repetition method :)

@bruAristimunha
Copy link
Collaborator

Thank you for the new dataset @gcattan!

@bruAristimunha bruAristimunha merged commit 58cc245 into NeuroTechX:develop Apr 20, 2023
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.

3 participants