Skip to content
New issue

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

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

Already on GitHub? # to your account

Enhance "Set reference" #258

Merged
merged 5 commits into from
Feb 7, 2022
Merged

Enhance "Set reference" #258

merged 5 commits into from
Feb 7, 2022

Conversation

hofaflo
Copy link
Contributor

@hofaflo hofaflo commented Feb 7, 2022

Currently, if the data set does not contain the reference channel, rereferencing requires to open "Set reference..." twice:

  • first, to add the reference channel
  • second, to actually rereference to the desired channel(s)

Example, following this tutorial:
image
image

Here's an idea what the dialog could look like to perform both steps at once (called "Modify reference" to reflect both possible actions):
image

This could be the initial state:
image

And only adding a reference channel, without rereferencing would still be possible:
image

@cbrnr
Copy link
Owner

cbrnr commented Feb 7, 2022

Yes, I like that. Some suggestions:

  • I would put "Set reference" first and "Add reference" second (and use those names)
  • You could try button groups instead of a checkbox with a label for both options. Button groups can also be checkable.
  • I'd use "Change reference" instead of "Modify reference"

@hofaflo
Copy link
Contributor Author

hofaflo commented Feb 7, 2022

Thanks for the suggestions, this is much nicer (to look at and to implement :D)
image

If I correctly understand what's happening, in case someone wants to both "Add" and "Set" something, the adding should happen first, right? Therefore I'd also put it first in the dialog 🤔

Edit: for future reference, the Qt element that makes this possible is a QGroupBox, not a QButtonGroup

@cbrnr
Copy link
Owner

cbrnr commented Feb 7, 2022

Yes, adding a reference channel (all zero) to the data should come before re-referencing to one or more existing electrodes. I thought that because people rarely do that, the option should go last. But you are right, it is probably better when you put it first (even though it is disabled by default).

I was thinking that "Set reference" and "Add reference" are very similar. We could use "Re-reference" instead of "Set reference" to highlight that this operation is applied to existing channels, whereas "Add reference" adds a new channel.

Do you perform any checks (e.g. adding a new reference requires a new name which is not equal to any existing channels name, and re-referencing requires one or more existing channel names)?

@hofaflo
Copy link
Contributor Author

hofaflo commented Feb 7, 2022

How about this, for maximum clarity?
image

ad checks: I was thinking mne performs those checks anyway, so mnelab could just show the error messages (and remove the data set duplicate, if applicable), like in epoch_data, wdyt?

@cbrnr
Copy link
Owner

cbrnr commented Feb 7, 2022

How about this, for maximum clarity?

👍

ad checks: I was thinking mne performs those checks anyway, so mnelab could just show the error messages (and remove the data set duplicate, if applicable), like in epoch_data, wdyt?

I didn't check, but yes, this would work of course! Theoretically we could use a (multi-selection) list widget that shows all existing channels instead of a text edit.

@hofaflo
Copy link
Contributor Author

hofaflo commented Feb 7, 2022

Theoretically we could use a (multi-selection) list widget that shows all existing channels instead of a text edit.

Not sure if that's a valid use case, but this wouldn't work if someone wanted to use a newly added channel as part of the rereferencing selection 🤔

Edit: but yes, this would probably be more practical :D

@cbrnr
Copy link
Owner

cbrnr commented Feb 7, 2022

Well, that's a very rare use case, and I think in such a case it is OK that the users opens the dialog twice.

@hofaflo
Copy link
Contributor Author

hofaflo commented Feb 7, 2022

Done :)

image

@cbrnr
Copy link
Owner

cbrnr commented Feb 7, 2022

I tried adding a reference and re-referencing to an existing channel, but got this error:

Traceback (most recent call last):
  File "/Users/clemens/Repositories/mnelab/mnelab/mainwindow.py", line 828, in change_reference
    ref = [c.strip() for c in dialog.reref_channellist.text().split(",")]
AttributeError: 'PySide6.QtWidgets.QListWidget' object has no attribute 'text'

@hofaflo
Copy link
Contributor Author

hofaflo commented Feb 7, 2022

Right, my bad, fixed now!

Copy link
Owner

@cbrnr cbrnr left a comment

Choose a reason for hiding this comment

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

Nice! I think this is good to go. Additional thought (maybe another PR): the "average" reference is identical to selecting all channels from the list, so we don't really need it. It's just a shortcut basically, and the most common use cases are either one or two channels or the average over all.

@cbrnr cbrnr merged commit 61e1ba9 into cbrnr:main Feb 7, 2022
@cbrnr
Copy link
Owner

cbrnr commented Feb 7, 2022

Thanks @hofaflo!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants