search

mne-tools / mne-qt-browser

A new backend for the 2D data browser in MNE-Python
BSD 3-Clause "New" or "Revised" License
39 stars 21 forks source link

New channel type scaling setting diffes from ViewBox scaling display #266

Open mscheltienne opened 3 days ago

mscheltienne commented 3 days ago

Describe the problem

The added channel type scaling in #263 is half of the display channel type. It would be nice to harmonize and choose which one to display.

Screenshot from 2024-07-02 15-55-18

@larsoner @nmarkowitz Any strong opinion here?

nmarkowitz commented 3 days ago

@mscheltienne I'm not sure what you mean by this

mscheltienne commented 3 days ago

grad (ft/cm) in the settings shows 400; mark on the viewbox shows 800 fT/cm.

nmarkowitz commented 3 days ago

ah I see. but that is also controlled by the independent mne.scale_factor property. Currently the formula for displaying that text is (self is a ScaleBarText instance and ch_type is any type of channel)

scaler = 1 if self.mne.butterfly else 2
inv_norm = (
    scaler
    * self.mne.scalings[self.ch_type]
    * self.mne.unit_scalings[self.ch_type]
    / self.mne.scale_factor
)
self.setText(f"{_simplify_float(inv_norm)} " f"{self.mne.units[self.ch_type]}")

The scale factor is adjusted by the eyeglass icons at the top of the browser. We can remove those and the user will have to control the scaling of each channel type manually. Or add a checkbox for whether to use the scale factor or purely use manual adjustment

Screenshot 2024-07-02 at 11 54 29 AM

larsoner commented 3 days ago

One option would be to get rid of self.mne.scale_factor completely and when the "scale all up" or "scale all down" buttons are changed, change the self.mne.scalings instead.

But that's a bit of a separate issue from whether or not the magenta scale bar matches the chosen scale factor. I think it probably should. In other words, it's twice as large as it should be at the moment, so we can just make it half the size (it could extend upward from the baseline/zero, or extend halfway above and halfway below the baseline).

nmarkowitz commented 3 days ago

Should this be fixed in https://github.com/mne-tools/mne-qt-browser/pull/268 or a separate PR?

larsoner commented 3 days ago

Could go either way... I would probably keep them separate until we converge on how to harmonize. In the meantime we can know that there should be a factor of 2x difference between the two when testing interactions in #268

mscheltienne commented 3 days ago

I would do 2 separate PRs

nmarkowitz commented 2 days ago

I fixed this in commit https://github.com/mne-tools/mne-qt-browser/pull/268/commits/ee2d1aba5188003323aac897ad327ec845b080ce . It was an issue with how I was calculating scale. I wasn't using complete formula. It's now synchronized.

I did find another error. When butterfly mode is toggled scale bar text isn't updated. I'll make a commit for that