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

Fix Brain slider ranges #12612

Merged
merged 3 commits into from
May 16, 2024
Merged

Fix Brain slider ranges #12612

merged 3 commits into from
May 16, 2024

Conversation

wmvanvliet
Copy link
Contributor

@wmvanvliet wmvanvliet commented May 16, 2024

This updates the logic that determines the ranges for the three sliders (fmin, fmid, fmax) on the left of a Brain figure. There was an overflow when the data is all zeros (np.log10(fmax)) which this PR fixes by using np.log10(max(fmax, 1e-30)) instead. While I was there, I noticed that the minimum and maximum values for the sliders were set based on the fmin and fmax of the current timepoint instead of the entire data. Finally, on second thought, the minimum value for the sliders should always be 0, that makes a lot more sense to me than capping it to fmin.

To check out the new behavior:

import mne
path = mne.datasets.sample.data_path()
stc = mne.read_source_estimate(path / "MEG/sample/fsaverage_audvis-meg")
brain = stc.plot("fsaverage", hemi="both", subjects_dir=path / "subjects")

image
image

fixes #12606

Copy link
Member

@drammock drammock left a comment

Choose a reason for hiding this comment

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

behavior does seem better, thanks! Please add a changelog entry.

@wmvanvliet wmvanvliet enabled auto-merge (squash) May 16, 2024 19:39
@drammock
Copy link
Member

once #12615 lands we can merge main and that should get CIs green here

@wmvanvliet wmvanvliet merged commit 5a20b82 into mne-tools:main May 16, 2024
30 checks passed
@wmvanvliet wmvanvliet deleted the slider-range branch May 17, 2024 06:03
@hoechenberger
Copy link
Member

hoechenberger commented May 17, 2024

image

i love it 😍

@larsoner
Copy link
Member

@wmvanvliet looks like this broke an example:

https://app.circleci.com/pipelines/github/mne-tools/mne-python/24096/workflows/69ebaf39-fe26-4774-83e0-238a1fd85918/jobs/66162

Unexpected failing examples:

    ../examples/inverse/source_space_snr.py failed leaving traceback:

    Traceback (most recent call last):
      File "/home/circleci/project/examples/inverse/source_space_snr.py", line 74, in <module>
        brain = snr_stc.plot(**kwargs)
      ...
      File "/home/circleci/project/mne/viz/_brain/_brain.py", line 4164, in _get_range
        fscale_power = int(np.log10(max(fmax, np.finfo("float32").min)))
    RuntimeWarning: invalid value encountered in log10

And locally just running the example I get the warning then an error:

/home/larsoner/python/mne-python/mne/viz/_brain/_brain.py:4164: RuntimeWarning: invalid value encountered in log10
  fscale_power = int(np.log10(max(fmax, np.finfo("float32").min)))
Traceback (most recent call last):
  File "/home/larsoner/python/mne-python/examples/inverse/source_space_snr.py", line 74, in <module>
    brain = snr_stc.plot(**kwargs)
            ^^^^^^^^^^^^^^^^^^^^^^
  ...
  File "/home/larsoner/python/mne-python/mne/viz/_brain/_brain.py", line 4164, in _get_range
    fscale_power = int(np.log10(max(fmax, np.finfo("float32").min)))
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

@wmvanvliet
Copy link
Contributor Author

Oh no! That looks like a platform issue. Was np.finfo("float32").min a poor choice?

@wmvanvliet
Copy link
Contributor Author

Yes, I really think it was a poor choice. I meant to do this: #12619

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

Successfully merging this pull request may close these issues.

OverFlowError when calling : SourceEstimate.plot(...)
5 participants