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

Feature bipolar references #173

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

gdolle
Copy link
Contributor

@gdolle gdolle commented Oct 16, 2020

  • Add a button to set bipolar references
  • Add a dialog to select channel by pair
  • Add the feature to save/open the selected channels to create bipolar reference profile (which (seems/) does not exist yet in MNE)

[updated]

@cbrnr
Copy link
Owner

cbrnr commented Oct 21, 2020

Thanks for your PR! Would you mind splitting it up so that one PR tackles exactly one issue?

Regarding the arguments, can you explain the issue with the current implementation and how your fix works?

Regarding the bipolar reference feature, currently you hard-code the derivations, but I'd prefer that the user decides which channels are anodes and which channels are cathodes. Based on this input, custom bipolar derivations could be computed.

Let me know what you think and if you need help implementing these changes!

@gdolle
Copy link
Contributor Author

gdolle commented Oct 21, 2020

Thanks for your PR! Would you mind splitting it up so that one PR tackles exactly one issue?

Yes ok I'll split in 2 PR.

Regarding the arguments, can you explain the issue with the current implementation and how your fix works?

I will explain in the specific new PR (and add a specific issue also)

Regarding the bipolar reference feature, currently you hard-code the derivations, but I'd prefer that the user decides which channels are anodes and which channels are cathodes. Based on this input, custom bipolar derivations could be computed.

I agree with you. Also maybe add a load/save button to create bipolar reference profile (to avoid setting every time)
I'll have a look (@Qdelannoy)

Let me know what you think and if you need help implementing these changes!

@gdolle
Copy link
Contributor Author

gdolle commented Nov 4, 2020

@cbrnr (@Qdelannoy) I updated this PR to select the channels via a dialog, you can give it a try ?

Base automatically changed from master to main January 17, 2021 09:29
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

3 participants