[ENH, MRG] Add object-oriented widget abstraction #10913
Conversation
|
Huh odd, it seems like maybe a new |
|
Except I think 3D rendering is broken in |
|
Ah I had to The one thing I still haven't figured out is simulating keypresses in |
|
This is ready for review besides the test failures which I am at a loss to explain; the code that they are executing is untouched, the new code is added on top in a way that should be completely compatible. Then there are some unrelated failures to load the headless display backend that also fail on |
|
I have a PR to pyvista currently to fix the mesh point error. It merged also so that should be fixed. |
|
Okay the failures look real/relevant now @alexrockhill |
Hmm, it passes locally for me. Maybe that shouldn't be tested on Windows? The code that gets the renderer and the renderer itself is completely untouched as far as I can tell so this is where I'm very confused about where the failure is coming from. |
It works on the existing backend so it should work on the new / refactored one
I would try adding some |
|
... or maybe better, get a Windows VM or boot running and test there, but that is some work though they are available: https://developer.microsoft.com/en-us/windows/downloads/virtual-machines/ Maybe this plus our latest windows installer would make this not too painful? |
|
I have access to a Windows machine and I tried to pip install and got an error with EDIT: Actually it did finish after about half an hour. But then I have mne version So it's been pretty painful so far... I'll try just pushing an empty commit again. Maybe something has changed in the last couple days because again I only added new classes, I changed it so that I didn't touch any of the old backend abstraction. I guess the thing I don't really understand is why this would be failing on |
|
Ah well it looks like there's all new unrelated test failures this time around |
|
Let's see how #10933 does |
|
Feel free to rebase or merge with main and push |
|
Ok I finally got it running locally on Windows and it fails but with an error past the current failure. Normally, I would 100% agree with you @larsoner that because the CI passed before I did anything and then failed when I changed the code that it's something in the code but the results are all over the place with this backend stuff so I really don't know what's going on or how to fix it.
|
|
Agreed in #10935 (comment) |
|
@larsoner, I think the failures are from the |
|
Huh okay so it looks like |
|
Hmm okay patched everything but clicks don't work for some reason in |
|
So the difference that is causing the failed tests is that |
So this is a problem in |
mne/viz/tests/test_raw.py
Outdated
| # see https://github.com/mne-tools/mne-python/issues/10961 | ||
| @pytest.mark.skipif( | ||
| sys.platform.startswith('win'), reason='Needs fix on Windows') |
There was a problem hiding this comment.
Yikes, it's all of these? I thought it was just notebook stuff that was broken
There was a problem hiding this comment.
Yeah it was 22 failed tests, I think about 12 were unique to parameterization. I have no idea why they are failing, I raised the issue in ipympl above because it seemed so odd. They work on Linux/MacOS so it seems like there is an underlying ipympl settings issue that causes odd things to happen. I can get on a Windows computer and have a look in 3 hours (in use right now).
There was a problem hiding this comment.
Yeah I assumed the ipympl stuff just meant that we would skip a couple of isolated notebook tests, but these are core viz tests. If ipympl installation breaks this many tests on Windows, we might be better off uninstalling ipympl from windows for now so we don't have to skip these tests.
There was a problem hiding this comment.
Hmm, okay, then notebook won't render matplotlib plots on Windows I think. I'll take a quick look at ipympl CIs and see if they test this kind of thing or what.
There was a problem hiding this comment.
Well that was quick, they don't have any CIs that run on Windows so I think that would be the issue. That really creates a headache for notebook support... as in, probably a bad idea to support the notebook backend on Windows when ipympl doesn't. I think beyond just uninstalling ipympl on Windows, there should be some documentation about this lack of support.
There was a problem hiding this comment.
I think the tests will pass without ipympl on Windows for this PR but I'm pretty sure the GUIs won't render properly. If we just do that in the CIs then the GUIs will work if someone installs via the requirements.txt file but these core viz tests fill fail and there could be more trouble. If we remove it from the requirements.txt, then maybe we should officially remove support for GUIs on Windows so that people know that it's not supposed to work. It's really bad that if someone just installs ipympl on Windows, the basic functionality of their MNE installation might break... That might be worth mentioning somewhere as well.
There was a problem hiding this comment.
I think the tests will pass without ipympl on Windows for this PR
Good!
but I'm pretty sure the GUIs won't render properly...
This is all the case on main, correct?
Let me iterate again -- if it's the case on main, then please let's deal with it in different PR where we figure out how to fix things properly. Adopting the new abstraction framework -- which this PR moves us toward -- is separable from how things work on windows, at least to the extent that things on windows are in the same state/probably broken with our current framework on main as well.
If we try to tackle these two problems simultaneously, then it creates a review nightmare, and prevents any new progress with a new framework (that you advocated for so strongly!). We could tackle the ipympl-and-testing-on-windows first and then come back to this PR if you really want, but then this PR is stuck and you can't use your new framework.
There was a problem hiding this comment.
Yeah, this should all be broken on main, it's just coming up because to get things to work properly for the abstraction, you need ipympl. I just pushed a commit that hopefully should pass, is that what you were thinking?
There was a problem hiding this comment.
Yes that's more or less the idea, but don't forget to remove all the skipifs since we don't need them now
There was a problem hiding this comment.
They're all gone in the diff.
I left the one on brain scraper because it seems like it's actually a separate issue; here's the CI where it alone failed (https://dev.azure.com/mne-tools/mne-python/_build/results?buildId=20714&view=logs&j=dded70eb-633c-5c42-e995-a7f8d1f99d91&t=8394206f-a00b-5cb7-44ca-464c9d5f9e25).
|
Ok, it took an extra 2 minutes and 45 seconds to uninstall |
|
Do you want to wait to merge this until after the release? Otherwise, I think this is okay now |
Any thoughts on a plan for this @larsoner? No need to rush merging just wanted to get your thoughts. |
|
We should release 1.1 in the next day or two so let's merge after that, then you can make use of this framework! |
|
Sounds good, okay to merge? |
|
Yes, thanks @alexrockhill ! |
There was a problem hiding this comment.
I won't pretend to have carefully reviewed / assessed the implementation, but the changes look reasonable and the code is pretty clear. Aside from a couple fairly minor comments/questions LGTM. I'll let @larsoner push the green button
| try: # remove when pyvista 0.35.2 patch is released | ||
| _close_all() | ||
| except AttributeError: | ||
| pass |
There was a problem hiding this comment.
pyvista 0.36.1 is latest release, do we still need this?
| _AbstractCanvas.__init__( | ||
| self, width=width, height=height, dpi=dpi) | ||
| self.fig = Figure(figsize=(width, height), dpi=dpi) | ||
| self.ax = self.fig.add_subplot(111, position=[0.15, 0.15, 0.75, 0.75]) |
There was a problem hiding this comment.
just curious where these position values come from, are they arbitrary? tailored to a specific kind of inset plot (i.e. the traces from clicked verts in a Brain GUI)? Wondering if we'll regret hard-coding this later, if some other plot type gets inserted here (e.g., I can easily imagine a time-freq image plot making sense in a Brain GUI, but potentially any others?)
There was a problem hiding this comment.
It's just to give more room on the edges so things don't get cut off. Tight layout doesn't play nicely with all the Qt stuff... Otherwise all the coordinate transforms should be handled by the axis and should be unaffected.
|
Haha, looks like @larsoner merged while I was reviewing. NBD, my comments were pretty minor anyway. |
|
@alexrockhill at least the PyVista thing is still relevant and I missed it, can you make a follow-up PR? |
Fixes #10779
Superceeds #10803
Follows @larsoner 's advice to split the abstraction up into a first PR and per conversation @drammock and @hoechenberger 's adds onto existing backend without replacing for now.