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

Minuit.interactive outside of Jupyter notebooks #1056

Open
wants to merge 44 commits into
base: develop
Choose a base branch
from

Conversation

adryyan
Copy link

@adryyan adryyan commented Nov 1, 2024

Implementation of a PyQt6 widget for minuit.interactive (Issue #771 ). In order to make the plotting work properly, I changed the API for the visualize methods of the build-in cost functions so that they optionally accept matplotlib Axes objects (and for CostSums a Figure). If these arguments don't receive a value, pyplot is used. This should keep the disruption of existing code as minimal as I could manage.

src/iminuit/qtwidget.py Outdated Show resolved Hide resolved
src/iminuit/minuit.py Outdated Show resolved Hide resolved
src/iminuit/qtwidget.py Outdated Show resolved Hide resolved
src/iminuit/qtwidget.py Outdated Show resolved Hide resolved
src/iminuit/qtwidget.py Outdated Show resolved Hide resolved
src/iminuit/qtwidget.py Outdated Show resolved Hide resolved
@HDembinski
Copy link
Member

@adryyan I reviewed this PR, but you also made another where you don't change the plotting API. I prefer the other one, as expressed here.

I think we need a few iterations, but I generally appreciate your effort very much. This is a significant contribution.

@adryyan
Copy link
Author

adryyan commented Dec 6, 2024

@HDembinski Thank you for your comments; I will go through them in the next weeks.

src/iminuit/qtwidget.py Outdated Show resolved Hide resolved
tests/test_draw.py Outdated Show resolved Hide resolved
@adryyan
Copy link
Author

adryyan commented Dec 12, 2024

I still don't understand: the associated ipywidget with that fit_button.click() is this:

iminuit/tests/test_draw.py

Lines 209 to 224 in 811b27b

with plot.assert_call():
out = m.interactive(raise_on_exception=True)
# this should modify slider range
ui = out.children[1]
header, parameters = ui.children
fit_button, update_button, reset_button, algo_select = header.children
assert parameters.children[0].slider.max == 1
assert parameters.children[1].slider.min == -1
with plot.assert_call():
fit_button.click()
assert_allclose(m.values, (100, -100), atol=1e-5)
# this should trigger an exception
plot.raises = True
with plot.assert_call():
fit_button.click()

In the interactive call raise_on_exception is set to true, so the ipywidget shouldn't catch the exception? Or am I missing something?

src/iminuit/minuit.py Outdated Show resolved Hide resolved
@HDembinski
Copy link
Member

In the interactive call raise_on_exception is set to true, so the ipywidget shouldn't catch the exception? Or am I missing something?

Ok, I will check this more thoroughly to give you a proper answer.

@adryyan adryyan changed the title Minuit.interactive outside of Jupyter notebooks (Changed cost visualize API) Minuit.interactive outside of Jupyter notebooks Dec 14, 2024
Comment on lines +1697 to +1698
except AttributeError:
return False
Copy link
Member

Choose a reason for hiding this comment

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

I think that is not necessary, the object returned by get_ipython always has the "has_trait" method.

Copy link
Author

Choose a reason for hiding this comment

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

I would have thought so as well, but when calling get_ipython() not in a jupyter notebook / ipython session, it just returns None and that of course has no attribute has_trait.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, then please add a comment since this is surprising.

@HDembinski
Copy link
Member

HDembinski commented Dec 16, 2024

@adryyan We are coming to the finish line. Thank you for all the hard work! This is a significant addition to iminuit, and I praise your improvements to the fitting GUI and your nice unit tests. I will mimic the new GUI in a separate PR at some later point in the ipywidget widget.

The last step is to make sure that all tests pass, then we can merge this PR. I took a glance, and the failures that I see seem minor and should be easy to fix.

@HDembinski
Copy link
Member

This one looks a bit tricky https://github.com/scikit-hep/iminuit/actions/runs/12353782099/job/34473645329?pr=1056#step:9:41
INTERNALERROR> ImportError: libEGL.so.1: cannot open shared object file: No such file or directory

If you cannot figure it out, I will handle that one.

@adryyan
Copy link
Author

adryyan commented Dec 16, 2024

Thank you for all your input, I learned some new things and I feel like the code is much cleaner now.

I don't really have any experience with nox, but the problems look to me like there are issues with the installed packages and importskips. I moved all tests regarding the qtwidget into one file and put the importskip of pyqt6 before the test functions, because of the qtbot (pytest-qt). I did the same with the ipywidget tests, because the mock_ipython fixture fails if there is no IPython. But I think the actions would also need to be updated to install the necessary packages, so that the qtwidget can be tested (PyQt6, pytest-qt), but I am not sure how to do that.

@HDembinski
Copy link
Member

I don't really have any experience with nox, but the problems look to me like there are issues with the installed packages

Yes, it should not have anything to do with nox. This looks like the image is missing a dependency. In fact, that error occurs in the doc build as well, but does not cause a failure:
https://github.com/scikit-hep/iminuit/actions/runs/12360486430/job/34524584092?pr=1056#step:8:80

The Test / X tests only check the basic functionality, that's why they do not expose this issue.

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.

2 participants