-
Notifications
You must be signed in to change notification settings - Fork 225
Dataset class for BrainForm by Romani et al. #825
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your nice contribution @BRomans !
Can you complete the wath s new file?
And the summary_p300.csv too?
I will ask you too, to test your dataset with an existing tutorial, for example this one
https://github.com/BRomans/moabb/blob/bromans/examples/paradigm_examples/plot_within_session_p300.py
And post the result in this thread.
Thank you.
… from session name accordingly
Signed-off-by: Bru <a.bruno@aluno.ufabc.edu.br>
moabb/datasets/romani_bf2025_erp.py
Outdated
| raw.load_data() | ||
|
|
||
| # Set montage (10–20 system) | ||
| montage = make_standard_montage(self.montage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is montage a parameter here? That's fine, but I have one comment to add: the rationale behind it needs to be included.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Unicorn allows replacement of the electrodes. In case additional datasets are collected with the same tool but different montage, it should be specified no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be the montage used for the current experiment.
If you plan to include similar dataset and reuse this code with different montages, this can be refactored later.
Co-authored-by: Bru <b.aristimunha@gmail.com> Signed-off-by: Michele Romani <BRomans@users.noreply.github.com>
Co-authored-by: Bru <b.aristimunha@gmail.com> Signed-off-by: Michele Romani <BRomans@users.noreply.github.com>
|
The download function needs to be refactored, here is one example of how to use the mne functions: In this example, it's doing the same thing as you: downloading a zip file and unzipping it. What you need to configure is the link, the code, etc. I'm giving you permission to run the CI; you can commit. If you encounter any errors, minor download issues, or anything unrelated to what you've been working on, you can ignore them. |
Co-authored-by: Bru <b.aristimunha@gmail.com> Signed-off-by: Michele Romani <BRomans@users.noreply.github.com>
Removed unused function Co-authored-by: Bru <b.aristimunha@gmail.com> Signed-off-by: Michele Romani <BRomans@users.noreply.github.com>
gcattan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BRomans were you able to run your example?
Please share in the thread the within session results.
…unused methods, created mapping between subjects IDs and their indexing for get_data() compatibility.
|
Hey @gcattan, I implemented the changes you requested and fixed a couple of bugs with the download. The warning
|
moabb/datasets/romani_bf2025_erp.py
Outdated
| raise FileNotFoundError(f"Dataset folder not found: {self.data_folder}") | ||
| return self.data_folder | ||
|
|
||
| def data_url( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be a private method.
moabb/datasets/romani_bf2025_erp.py
Outdated
|
|
||
| return subjects | ||
|
|
||
| def get_session_list(self, subject): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don t think we need this. Can you remove please?
|
@BRomans thanks for sharing the results.
Ok, but we need a fix for this. Either to add correct stimulation channel, either to ignore the warning. |
|
@gcattan I understand your concern. However, I do not believe it to be a very elegant solution. The problem lies in this function, called by Basically, it assumes that annotations must be recreated from stim channel. Thus, even if I update the values of the triggers to match the convetion 1:"Target" and 2:"NonTarget", the newly created annotations do no respect the naming and the pipeline crashes. |
|
@BRomans sorry I don t get it. |
|
@gcattan I implemented as you suggested. This is the output |
gcattan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Thank you @BRomans !
@bruAristimunha LGTM?
|
Amazing work @BRomans 🙏🏽 |
|
many thank you for the revision @gcattan |
|
Thank you both for the support! @bruAristimunha @gcattan |
Implementation to load the BrainForm dataset from BIDS files.
Works with both locally stored files if path is provided, otherwise fetches the dataset from Zenodo.
Local files can also be newer sessions collected with the BrainForm neurogame and converted to BIDS format.