-
Notifications
You must be signed in to change notification settings - Fork 22
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
channel scaling spinboxes update #268
channel scaling spinboxes update #268
Conversation
@nmarkowitz just let me know when I should take a look / review / try it! |
mne_qt_browser/_pg_figure.py
Outdated
self.mne.scalings[ch_type] | ||
* self.mne.unit_scalings[ch_type] | ||
* scaler | ||
/ self.mne.scale_factor |
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...
mne_qt_browser/_pg_figure.py
Outdated
self.mne.scalings[ch] | ||
* self.mne.unit_scalings[ch] | ||
* scaler | ||
/ self.mne.scale_factor |
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.
... and this are identical (as are the scaler
computations). Better to refactor into a helper I think as I commented on one of your commits
mne_qt_browser/_pg_figure.py
Outdated
new_value = args[0] | ||
ch_type = kwargs["ch_type"] | ||
self.mne.scalings[ch_type] = new_value / ( | ||
self.mne.unit_scalings[ch_type] * scaler / self.mne.scale_factor |
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.
... and this could use the same helper but with some divisions probably?
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.
Added helper in 5eea6c6
@nmarkowitz when I change the channel scalings, the scale bar updates (good) but the channel scaling does not change -- the traces should get bigger and smaller as I click up and down: Peek.2024-07-08.15-48.1.mp4Compare this to when I use the existing +/- functionality (which appears to update properly): Peek.2024-07-08.15-50.1.mp4Do you see the same thing locally? If so, make sure you test interactively like this in addition to writing unit tests -- arguably it's a more important test to do anyway! |
@larsoner it works I just made the step size too small. Now it's larger and should be more noticeable |
Not for me -- here I'll change from 800 to 8000 which is a factor of 10, so the traces should get a factor of 10 smaller, but they don't change: Peek.2024-07-08.17-20.1.mp4The traces get smaller/larger for you when you click the up/down arrows in the double spin box? |
@larsoner it works for me Screen.Recording.2024-07-09.at.9.48.32.AM.movCould you be using a different qt package such as PySide? I'm using PyQt5 |
Yeah I'm using PyQt6. I just tried with PySide6 and observed the same thing. So maybe it's a Qt5 vs Qt6 thing? It's weird, not sure how to explain it. |
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.
Just some minor comments.
For the PyQt stuff maybe try creating a conda env with pyside6
?
mne_qt_browser/_pg_figure.py
Outdated
ordered_types = self.mne.ch_types[self.mne.ch_order] | ||
unique_type_idxs = np.unique(ordered_types, return_index=True)[1] | ||
ch_types_ordered = [ordered_types[idx] for idx in sorted(unique_type_idxs)] | ||
for ch in ch_types_ordered: | ||
if ch in self.mne.unit_scalings.keys(): | ||
ch_spinbox = QDoubleSpinBox() | ||
ch_spinbox.setMinimumWidth(100) | ||
ch_spinbox.setRange(-float("inf"), float("inf")) |
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 you should be able to set negative values. Zero is probably bad, too, but this is at least a step in the right direction:
ch_spinbox.setRange(-float("inf"), float("inf")) | |
ch_spinbox.setRange(0, float("inf")) |
ch_spinbox.setDisabled(True) | ||
# ch_spinbox.setReadOnly(True) | ||
ch_spinbox.setMinimumWidth(150) | ||
ch_spinbox.setDecimals(1) |
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.
Maybe 2 is better and setting to an adaptive step type could help?
mne_qt_browser/_pg_figure.py
Outdated
ch_spinbox.setMinimumWidth(150) | ||
ch_spinbox.setDecimals(1) | ||
inv_norm = _get_channel_scaling(self, ch) | ||
ch_spinbox.setSingleStep(inv_norm * 0.25) |
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.
Have you tried adaptive, might work decently?
https://doc.qt.io/qt-6/qdoublespinbox.html#setStepType
https://doc.qt.io/qt-6/qdoublespinbox.html#stepType-prop
mne_qt_browser/_pg_figure.py
Outdated
if self.mne.fig_settings is not None: | ||
self.mne.fig_settings._update_spinbox_values() | ||
|
||
# self._update_ch_spinbox_values() |
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.
Cruft, remove?
@larsoner I just created a new environment that uses pyside6 and the spinboxes still work for me. I even tried running in a regular python console as opposed to an ipython terminal. Not sure why it isn't working for you |
Thanks @nmarkowitz ! |
Reference issue
#230 , mne-tools/mne-python#10888 , #212
What does this implement/fix?
Spinboxes in channel scalings within Settings can now be updated
Additional information
This is the second of 4 or 5 PRs to allow display and scaling of channel types based on sensor units and monitor resolution and size