Enh: channel sensitivity ui

[nmarkowitz]

Reference issue

#230 , #212 , #208 , mne-tools/mne-python#10888

What does this implement/fix?

Adds widget to settings dialog displaying units of sensor in relation to screen size. Ability to edit widgets will be done in future pull request

[larsoner]

Rather than another set of rows with the channel types duplicated it probably makes more sense as a grid layout like

Type	Scaling	Sensitivity
grad (fT/cm)	800 ↕	111 ↕
mag (fT)	2000 ↕	277.5 ↕
eeg (uV)	40 ↕	5.5 ↕
eog (uV)	300 ↕	41.6 ↕
		/ mm ⌄

[nmarkowitz]

Rather than another set of rows with the channel types duplicated it probably makes more sense as a grid layout like

Type Scaling Sensitivity
grad (fT/cm) 800 ↕ 111 ↕
mag (fT) 2000 ↕ 277.5 ↕
eeg (uV) 40 ↕ 5.5 ↕
eog (uV) 300 ↕ 41.6 ↕
/ mm ⌄

I like this idea. Just pushed it

[larsoner]

I like this idea. Just pushed it

Well not quite... you still have redundant channel type names and units (other than the "per distance") in there, and they don't line up in rows either:

In other words, you did something like this rather than what I suggested:

Scalings		Sensitivities
grad (fT/cm)	800	Physical units	mm
mag (fT)	2000	grad (fT/cm/mm)	93.6

I think my suggestion is more compact / consistent but contains all the same information. Can you see if my suggestion works?

By eye looking at the cm it appeared to be correct, but you'll need to now also connect it to the resize event so that the values update when you resize. (The way I wanted to check the correctness was to resize my window such that the scale bar was very close to 1cm then check that the scale bar matched the Sensitivities value, but the spinbox values didn't update when I changed my window size.)

[nmarkowitz]

How does this look now? This is what was is your original suggestion. I'm wondering if the dropdown should be placed elsewhere. And for the next PR we're probably gonna need some toggle button like a checkbox or dropdown to choose which units are displayed on the scalebars

[larsoner]

@nmarkowitz I resized my window to show 800 (fT/cm)/cm so that the MEG scale bar should be 1cm but it was a bit big. Adjusting the __init__ of class ScaleBar to use a flat instead of rounded cap (to not draw farther than we should) seemed to fix it:

        pen = self.mne.mkPen(color="#AA3377", width=5)
        pen.setCapStyle(Qt.FlatCap)
        self.setPen(pen)

However, you should not need to know anything about the actual data values to know the effective scale on screen. So using data at all (and derivatives like peak_to_peak_data) like you do here should not be needed. The only thing that should matter are:

1. The factors that are used to scale the data
2. The number of pixels used to / allocated to show those data (which you should get from the viewbox somehow, could be correctly below?), which you can get from the proportion of the viewbox (in pixels) that is dedicated to showing the data

From those two things you should be able to figure out the effective scaling. Unpacking your code later you have (for mm at least):

        pixels_per_data_unit = pixel_height / data_height
        peak_to_peak_pixels = peak_to_peak_data * pixels_per_data_unit
        dpi = QApplication.primaryScreen().logicalDotsPerInch()
        peak_to_peak_inches = peak_to_peak_pixels / dpi
        peak_to_peak_mm = peak_to_peak_inches * 25.4
        if units == "mm":
            return (
                _get_channel_scaling(widget, ch_type)
                * peak_to_peak_data
                / peak_to_peak_mm

which is equivalent to

        dpi = QApplication.primaryScreen().logicalDotsPerInch()
        peak_to_peak_mm = peak_to_peak_data * pixel_height / data_height / dpi * 25.4
        if units == "mm":
            return (
                _get_channel_scaling(widget, ch_type)
                * peak_to_peak_data
                / peak_to_peak_mm
            )

which in turn is equivalent to

        dpi = QApplication.primaryScreen().logicalDotsPerInch()
        if units == "mm":
            return (
                _get_channel_scaling(widget, ch_type)
                / (pixel_height / data_height / dpi * 25.4)
            )

I'll push a commit to simplify the calculation once I verify it's correct

[larsoner]

Looks good, pushed a commit!

As usual, next I think some tests would be good. For example you could just get whatever the current scalings values are, change the units to / cm or something and then query the double spin box values and make sure they changed by the correct amount. You could also resize the window by 10% for example and make sure the values change by 10%.

[larsoner]

I just did:

mne browse_raw ~/mne_data/MNE-sample-data/MEG/sample/sample_audvis_raw.fif

and then opened the settings dialog and pressed "b" to check to see if the values updated properly when butterfly mode was activated (because this changes the scalebar size). Have you checked? I get an error:

$ mne browse_raw ~/mne_data/MNE-sample-data/MEG/sample/sample_audvis_raw.fif
Traceback (most recent call last):
  File "/home/larsoner/python/mne-qt-browser/mne_qt_browser/_pg_figure.py", line 5071, in keyPressEvent
    slot()
  File "/home/larsoner/python/mne-qt-browser/mne_qt_browser/_pg_figure.py", line 4903, in _toggle_butterfly
    self._set_butterfly(not self.mne.butterfly)
  File "/home/larsoner/python/mne-qt-browser/mne_qt_browser/_pg_figure.py", line 4856, in _set_butterfly
    self._update_ch_spinbox_values()
  File "/home/larsoner/python/mne-qt-browser/mne_qt_browser/_pg_figure.py", line 4007, in _update_ch_spinbox_values
    self.mne.fig_settings.update_all_spinboxes()
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'SettingsDialog' object has no attribute 'update_all_spinboxes'

Assuming you can replicate, once you fix the bug you can to make sure the values update properly when activating/deactivating butterfly mode. But keep in mind #276 where I point out that butterfly mode is already perhaps somewhat broken. It might be hard for you even to measure the size of the scale bar given the overlap... but you can at least eyeball the top half of the scale bar for the topmost channel type and go based on that I think.

[nmarkowitz]

Thanks for catching that. Just pushed commit

[nmarkowitz]

@larsoner every failed test is due to the sensitivity unit test I added. It passes on my computer but I don't know how to make it pass on github. Maybe I should change what the unit test is?

[larsoner]

It could be some little precision issue, I think it's okay to use assert_allclose(a, b, atol=0.1) to tolerate the rounded term being different

[nmarkowitz]

Just pushed using assert_allclose() for unit test. I set tolerance for 2.

[larsoner]

1. Did you measure to check the values? When I plot set the window size to match the stated sensitivity in inches:

   

   The physical measurement shows closer to 1/4" from the top of the "grad" scale bar to where the "grad" text is, suggesting that its full size is 1/2" for the 800 fT/cm. So I think there's a factor of two error?

2. From this view, when I hit "b" the plot updates but the spinbox values don't update:

   

   If I trivially resize the window e.g. in the horizontal direction, they do:

   

   So I think the "spin box update when butterfly mode is toggled" still needs to be updated / connected.

[nmarkowitz]

I fixed the butterfly toggle.

For the butterfly sensitivity I think it's accurate. It's difficult to tell with overlapping scalebars. I tried measuring it with just one channel type on the screen. Though it could have been due to a prior error.

[larsoner]

Now I have a bug where if I do:

$ mne browse_raw ~/mne_data/MNE-sample-data/MEG/sample/sample_audvis_raw.fif --clipping None --butterfly

This one looks correct, the distance from the top of the grad bar to the grad ylabel tick is very close to 1/4" so the full bar (assuming it's centered) is 1/2", and the scale bar is labeled as 400 fT/cm in the plot and the spin boxes, suggesting the sensitivity is 400 fT/cm/half-inch, which is the same as 800 fT/cm/inch (i.e., the spin box is correct correct):

But if I do the same command without the --butterfly then interactively do "b" on the keyboard to toggle it, I get the same plot and same spin box values as before except text on the scale bar says 800 fT/cm despite the text in the spin boxes being update:

So I think there's some bug with updating the scale bar values perhaps, but I think we can chalk this up to #276. Might be a good one to tackle next if you want @nmarkowitz or you can move on to making the sensitivity boxes interactive (I expect this one might be easy now hopefully)!

[larsoner]

Just a few cruft cleanups which I'll commit then mark for merge-when-green, thanks in advance @nmarkowitz !

FYI to spot these things, after you think your code is good for review and you push, I typically try to add one more step before pinging for re-review -- looking briefly at the diff on GitHub in the "Files" tab like https://github.com/mne-tools/mne-qt-browser/pull/273/files. A quick 30-sec scan of your own code probably would have caught these leftover lines to be removed!