Skip to content

FIX: focalpoint #9010

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

Merged
merged 9 commits into from
Apr 23, 2021
Merged

FIX: focalpoint #9010

merged 9 commits into from
Apr 23, 2021

Conversation

GuillaumeFavelier
Copy link
Contributor

This PR fixes #9007 by changing the camera's default focal point.

@GuillaumeFavelier GuillaumeFavelier self-assigned this Mar 8, 2021
# focalpoint: if 'auto', we use the center of mass of the visible
# bounds, if None, we use the existing camera focal point otherwise
# we use the values given by the user
if focalpoint == 'auto':
Copy link
Member

Choose a reason for hiding this comment

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

>>> np.array([0, 0, 0]) == 'auto'
<stdin>:1: FutureWarning: elementwise comparison failed; returning scalar instead, but in the future will perform elementwise comparison
False

Typically we handle this with:

_validate_type(focalpoint, (str, None, np.ndarray), 'focalpoint')
if isinstance(focalpoint, str):
    _check_option('focalpoint', focalpoint, ('auto',), extra='when a string')
    ...
elif focalpoint is None:
    ...
else:
    ...

@larsoner
Copy link
Member

larsoner commented Mar 8, 2021

@GuillaumeFavelier
Copy link
Contributor Author

Those are really scary. I think it got reported but I didn't take care of it yet. I agree it's probably resize event or something.

My next focus will be between this and multiplotter.

@larsoner
Copy link
Member

@GuillaumeFavelier can you push a commit with [circle full] and then we can look to see if things look okay?

@GuillaumeFavelier
Copy link
Contributor Author

BTW, the results are available on:

https://26478-1301584-gh.circle-artifacts.com/0/dev/index.html

@GuillaumeFavelier
Copy link
Contributor Author

@GuillaumeFavelier GuillaumeFavelier changed the title Fix: focalpoint WIP,FIX: focalpoint Apr 8, 2021
@GuillaumeFavelier
Copy link
Contributor Author

Circle probably uses PyVista master since it breaks already. I'll open a PR to get ready for those changes.

@drammock drammock mentioned this pull request Apr 18, 2021
@larsoner larsoner added this to the 0.23 milestone Apr 19, 2021
@larsoner
Copy link
Member

@GuillaumeFavelier any chance to return to this? It's going to be a blocker for 0.23

@GuillaumeFavelier
Copy link
Contributor Author

TBH I was counting on #8997 to fix it indirectly but considering the slow progress, it might not be enough for the next release. I'll give it max priority then 👍

@larsoner
Copy link
Member

I think #8997 is a good one to merge right after release rather than right before it. It's a big change and we want some months of testing and use ourselves to make sure it's stable

@GuillaumeFavelier
Copy link
Contributor Author

I think it's worth a [circle full] after #9318 is merged.

@GuillaumeFavelier
Copy link
Contributor Author

tutorials/forward/10_background_freesurfer.py: https://27738-1301584-gh.circle-artifacts.com/0/dev/auto_tutorials/forward/10_background_freesurfer.html?highlight=freesurfer

main PR
image image

tutorials/forward/50_background_freesurfer_mne.py: https://27738-1301584-gh.circle-artifacts.com/0/dev/auto_tutorials/forward/50_background_freesurfer_mne.html?highlight=freesurfer

main PR
image image

For reference:

examples/connectivity/sensor_connectivity.py: https://27738-1301584-gh.circle-artifacts.com/0/dev/auto_examples/connectivity/sensor_connectivity.html?highlight=sensor%20connectivity
image

And locally for tutorials/inverse/60_visualize_stc.py:

image
image
image

@larsoner
Copy link
Member

LGTM -- ready to go, then?

@GuillaumeFavelier
Copy link
Contributor Author

Ready on my end

@GuillaumeFavelier GuillaumeFavelier changed the title WIP,FIX: focalpoint FIX: focalpoint Apr 23, 2021
Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

LGTM will merge once CIs come back happy!

@larsoner larsoner merged commit 4f0ba23 into mne-tools:main Apr 23, 2021
@larsoner
Copy link
Member

Thanks @GuillaumeFavelier !

@GuillaumeFavelier GuillaumeFavelier deleted the fix/focalpoint branch April 23, 2021 15:03
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.

Fix corrupted figures in plot_background_freesurfer_mne
2 participants