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

MNT: Add support for QtPy #104

Merged
merged 43 commits into from
Apr 11, 2022
Merged

Conversation

GuillaumeFavelier
Copy link
Contributor

@GuillaumeFavelier GuillaumeFavelier commented Apr 7, 2022

This PR adds support for qtpy.

TODO:

Then this PR is ready to go

@larsoner
Copy link
Member

larsoner commented Apr 7, 2022

We should also port over the Qt job from MNE-Python https://github.com/mne-tools/mne-python/blob/main/azure-pipelines.yml#L147

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.

Looks like a good start so far!

@larsoner
Copy link
Member

larsoner commented Apr 8, 2022

... actually all the CI jobs here are GH actions, so I would just add a new entry in qt_viz_tests.yml

@larsoner
Copy link
Member

larsoner commented Apr 8, 2022

@larsoner
Copy link
Member

larsoner commented Apr 8, 2022

I pushed a commit that at least made things work for me on PyQt5 again -- widget.screen() is only available on 5.14+.

On PySide6 I get this ugly error:

(qt6) $ mne browse_raw ~/mne_data/MNE-sample-data/MEG/sample/sample_audvis_raw.fif 
Traceback (most recent call last):
...
File "/Users/larsoner/python/mne-python/mne/viz/_figure.py", line 613, in _load_backend
    from mne_qt_browser import _pg_figure as backend
  File "/Users/larsoner/python/mne-qt-browser/mne_qt_browser/_pg_figure.py", line 2620, in <module>
    class MNEQtBrowser(BrowserBase, QMainWindow, metaclass=_PGMetaClass):
  File "/Users/larsoner/mambaforge/envs/qt6/lib/python3.9/abc.py", line 106, in __new__
    cls = super().__new__(mcls, name, bases, namespace, **kwargs)
TypeError: Shiboken.ObjectType.__new__(_PGMetaClass) is not safe, use type.__new__()

It has something to do with the _PGMetaClass stuff, not sure how to fix it :(

@larsoner
Copy link
Member

larsoner commented Apr 8, 2022

Wow, green! Locally when trying any keyboard shortcut, though, I get things like:

Traceback (most recent call last):
  File "/Users/larsoner/python/mne-qt-browser/mne_qt_browser/_pg_figure.py", line 4331, in keyPressEvent
    'Ctrl': '4' in hex(int(event.modifiers())),
TypeError: int() argument must be a string, a bytes-like object or a number, not 'KeyboardModifier'
Traceback (most recent call last):
  File "/Users/larsoner/python/mne-qt-browser/mne_qt_browser/_pg_figure.py", line 4331, in keyPressEvent
    'Ctrl': '4' in hex(int(event.modifiers())),
TypeError: int() argument must be a string, a bytes-like object or a number, not 'KeyboardModifier'

@GuillaumeFavelier I'll stop here for now, feel free to continue when you want :)

@larsoner larsoner mentioned this pull request Apr 8, 2022
@larsoner
Copy link
Member

larsoner commented Apr 8, 2022

Modifier bugs were present in main actually, so I fixed them in #109

@larsoner
Copy link
Member

larsoner commented Apr 8, 2022

Pushed another little commit to take care of the warnings due to using 'AnyStyle':

qt.qpa.fonts: Populating font family aliases took 198 ms. Replace uses of missing font family "AnyStyle" with one that exists to avoid this cost. 

@larsoner larsoner marked this pull request as ready for review April 9, 2022 13:55
@larsoner larsoner changed the title WIP,MNT: Add support for QtPy MNT: Add support for QtPy Apr 9, 2022
@larsoner
Copy link
Member

larsoner commented Apr 9, 2022

FYI I need mne-tools/mne-python#10508 for macOS + Python3.10. I'll see if I can replicate on CIs...

@larsoner
Copy link
Member

larsoner commented Apr 9, 2022

Okay, PySide6, PyQt6, and PyQt5 all work on my macOS system!

@larsoner larsoner changed the title MNT: Add support for QtPy WIP,MNT: Add support for QtPy Apr 9, 2022
@larsoner
Copy link
Member

larsoner commented Apr 9, 2022

Setting back to WIP because, even though it's getting close, mne-tools/mne-python#10509 should go in first (which should also go in after mne-tools/mne-python#10500)

Comment on lines +397 to -399
times = times[::self.mne.decim_data[self.range_idx]]
data = data[..., ::self.mne.decim_data[self.range_idx]]
else:
times = self.mne.times
Copy link
Member

@larsoner larsoner Apr 9, 2022

Choose a reason for hiding this comment

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

This "caching" of the decimated times was causing problems with tests locally. Creating views (which slicing does) is a 100 ns-order operation:

In [1]: import numpy as np
In [2]: x = np.random.RandomState(0).randn(10000)
In [3]: %timeit x[100:1111]
136 ns ± 3.88 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

So let's not bother, and instead just slice times at the same time / way as the data

@larsoner
Copy link
Member

larsoner commented Apr 9, 2022

Okay @GuillaumeFavelier I think I'm actually done now :)

Feel free to review sometime next week and we can get this in once mne-tools/mne-python#10509 is merged

@larsoner larsoner changed the title WIP,MNT: Add support for QtPy MNT: Add support for QtPy Apr 11, 2022
@larsoner
Copy link
Member

@agramfort or @hoechenberger feel free to review and merge if it works for you on PyQt6 and/or PySide6

@agramfort agramfort merged commit 86d604a into mne-tools:main Apr 11, 2022
@agramfort
Copy link
Member

thx @GuillaumeFavelier @larsoner it works like a charm !

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.

3 participants