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

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

Open
mscheltienne opened this issue Jul 2, 2024 · 8 comments
Open
Labels
enhancement New feature or request

Comments

@mscheltienne
Copy link
Member

mscheltienne commented Jul 2, 2024

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?

@mscheltienne mscheltienne added the enhancement New feature or request label Jul 2, 2024
@mscheltienne mscheltienne changed the title Channel type scaling New channel type scaling setting vs ViewBox display Jul 2, 2024
@mscheltienne mscheltienne changed the title New channel type scaling setting vs ViewBox display New channel type scaling setting diffes from ViewBox scaling display Jul 2, 2024
@nmarkowitz
Copy link
Contributor

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

@mscheltienne
Copy link
Member Author

mscheltienne commented Jul 2, 2024

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

@nmarkowitz
Copy link
Contributor

nmarkowitz commented Jul 2, 2024

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
Copy link
Member

larsoner commented Jul 2, 2024

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
Copy link
Contributor

Should this be fixed in #268 or a separate PR?

@larsoner
Copy link
Member

larsoner commented Jul 2, 2024

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
Copy link
Member Author

I would do 2 separate PRs

@nmarkowitz
Copy link
Contributor

I fixed this in commit ee2d1ab . 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants