Skip to content

Conversation

@GuillaumeFavelier
Copy link
Contributor

This PR is extracted from #10224:

  • fixes the spin box "double call"

@GuillaumeFavelier GuillaumeFavelier self-assigned this Jan 26, 2022
@hoechenberger
Copy link
Member

Daaaamn, all of this just for a spinbox? 😳

@GuillaumeFavelier
Copy link
Contributor Author

Maybe there is simpler... This is only what I suggest and tested locally 😅

@GuillaumeFavelier
Copy link
Contributor Author

What do you think @larsoner ?

@larsoner
Copy link
Member

Why is this necessary? On main if I add a print(f'set {value} {mode_name} {coord}') as the first line in _set_parameter_safe, I already only get one value change per click I think (note that the "trans not shown at start" bug is still present on main):

$ mne coreg -s sample -d subjects -f MEG/sample/sample_audvis_raw.fif --trans MEG/sample/sample_audvis_raw-trans.fif
Peek.2022-01-26.10-11.mp4

What I don't like is that the display seems to update twice, which I assume has to do with an errant update=True somewhere or something...

@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Jan 26, 2022

You do not have this behaviour... Is it only me then?

main
One click -> From 0.00 to -2.00:

main.mp4

PR
One click -> From 0.00 to -1.00:

pr.mp4

@larsoner
Copy link
Member

No, I click once and it only updates once. Are you on an older PyQt5? I'm on 5.15. maybe it's a 5.12 bug for example.

Did you try the simpler suggested solution of:

Simplest: change all connect() to QSpinbox click/key to pass Qt::QueuedConnection as last parameter for connection type, not default Qt::DirectConnection.

?

@larsoner
Copy link
Member

Oh you said in #10244

I tried using Qt.ConnectionType.QueuedConnection with PyQt5 5.15.6 but it still did not fix the issue locally.

It's too bad this didn't work :(

@GuillaumeFavelier
Copy link
Contributor Author

I am using PyQt5 5.15.6:

❯ python -c "import mne; mne.sys_info()"
Platform:       Linux-4.19.223-1-MANJARO-x86_64-with-glibc2.17
Python:         3.8.12 (default, Oct 12 2021, 13:49:34)  [GCC 7.5.0]
Executable:     /home/guillaume/source/anaconda3/envs/mne-py38/bin/python
CPU:            : 4 cores
Memory:         7.7 GB

mne:            1.0.dev0
numpy:          1.22.0 {}
scipy:          1.7.1
matplotlib:     3.5.1 {backend=QtAgg}

sklearn:        1.0
numba:          Not found
nibabel:        3.2.1
nilearn:        0.8.1
dipy:           1.4.1
cupy:           Not found
pandas:         1.3.3
pyvista:        0.32.0 {OpenGL 4.5.0 NVIDIA 495.44 via NVIDIA GeForce GTX 960M/PCIe/SSE2}
pyvistaqt:      0.8.dev0
ipyvtklink:     0.2.1
vtk:            9.1.0
PyQt5:          5.15.6
ipympl:         Not found
mne_qt_browser: Not found
pooch:          v1.5.2

I tried connecting the spin boxes with QueuedConnection two days ago but it did not solve the issue.

@larsoner
Copy link
Member

Every time we parallelize something we make debugging harder, but this seems to be a real bug on your system at least so I guess we have to live with the complexity. Feel free to merge if it does fix things for you @GuillaumeFavelier , I at least notice no change in behavior (in a good way) on my system.

@GuillaumeFavelier
Copy link
Contributor Author

Okay, but the other PRs have higher priority. Now that I know that this could be related to my system, I think I'll hold on a bit before merging.

I can try:

  • a fresh new conda env
  • a different version of PyQt5
  • a different OS (on the same system)

@larsoner
Copy link
Member

If you hit the bug, and it's a known behavior of Qt under some circumstances (based on the threads), it's at least somewhat likely some user will have this problem. We don't want to make them mess with Qt installs as a fix...

@GuillaumeFavelier
Copy link
Contributor Author

We don't want to make them mess with Qt installs as a fix...

Indeed not optimal

Okay fair enough, I'll merge then.

@GuillaumeFavelier GuillaumeFavelier merged commit e1aec0e into mne-tools:main Jan 26, 2022
@GuillaumeFavelier GuillaumeFavelier deleted the fix/coreg_double_call branch January 26, 2022 16:02
@hoechenberger
Copy link
Member

Amazing detective work, @GuillaumeFavelier!

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