-
-
Notifications
You must be signed in to change notification settings - Fork 5k
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
TST: fix CI failures in test_all_nograd_minimizers
#20699
Conversation
Thanks! This would work, but the test took only 3s before adding COBYQA, and now it takes over 20s. Is it that slow locally? It might be worth a look at what's going on. |
@@ -212,6 +212,7 @@ def test_all_minimizers(self): | |||
niter=self.niter, disp=self.disp) | |||
assert_almost_equal(res.x, self.sol[i], self.tol) | |||
|
|||
@pytest.mark.fail_slow(40) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, the test runs this fast locally on an i9-13900K
:
5.13s call build-install/lib/python3.11/site-packages/scipy/optimize/tests/test__basinhopping.py::TestBasinHopping::test_all_nograd_minimizers
If I do Matt's suggested experiment of parametrization, the timings drop off pretty fast after that new method.
4.43s call build-install/lib/python3.11/site-packages/scipy/optimize/tests/test__basinhopping.py::TestBasinHopping::test_all_nograd_minimizers[COBYQA]
0.15s call build-install/lib/python3.11/site-packages/scipy/optimize/tests/test__basinhopping.py::TestBasinHopping::test_all_nograd_minimizers[COBYLA]
0.13s call build-install/lib/python3.11/site-packages/scipy/optimize/tests/test__basinhopping.py::TestBasinHopping::test_all_nograd_minimizers[CG]
0.08s call build-install/lib/python3.11/site-packages/scipy/optimize/tests/test__basinhopping.py::TestBasinHopping::test_all_nograd_minimizers[TNC]
0.07s call build-install/lib/python3.11/site-packages/scipy/optimize/tests/test__basinhopping.py::TestBasinHopping::test_all_nograd_minimizers[BFGS]
If you did parametrize, I believe you could use pytest.param
to apply the timing exception or another marker to just that one test case, so that other regressions don't sneak in. If you apply xslow
, then seems like it would only make sense to apply it to the one case that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tolerance to pass the test is quite coarse, though. Can the termination settings be adjusted to avoid unnecessary iterations? Or why else is the new method so much slower than the others on this problem?
I wonder why cobyqa is taking much longer than the others? @ragonneau, is there something about the architecture of the minimiser that leads to longer runtime? |
To get this passing, how about But it would also be good to see why the wall clock time is so high. |
The objective function used in that tests might just be a pathological edge case for |
Looks like the latest commit didn't improve the situation. I'm not sure I like having a test fail in CI because it takes too long. It seems like a recipe for flaky CI performance. Performance of CI runners can be variable sometimes, so we might get random fails. If the aim is to reduce time used by CI we'd be better off working on other areas. Lastly, it'd be nice to know why cobyqa takes so long for this specific test. |
I'm not sure that e.g.
If I do a naive patch to skip the code that has no effect (because I'm doing an N=10 |
[skip cirrus] [skip circle]
Improving COBYQA's performance would be the best option but take a longer time. Until then we should fix this annoying failure still. |
The iteration reduction brought the duration from over 20s to 5s - but just a hair over 5s, which is why the test still failed. I increased the time limit to 10s on this test to fix CI, but yeah, I think it's worth looking deeper into why it's so slow. Having slow tests fail is an experiment. Let's let it run for a while. This failure came up in main only because CI didn't run on the COBYQA PR after the FWIW, I don't necessarily think that this is the best way to keep CI time down either. I just don't like it when my tests are in the spotlight for being too slow (e.g. gh-13859, gh-20420), so I thought I'd try to fix the problem once and for all for everyone rather than putting out fires every once in a while. |
Hi everyone, Sorry for my very late reply, I am recently very busy with some personal matters. First of all, thank you everyone for looking into this matter. We designed COBYQA for solving DFO problems, which usually requires several minutes/hours/days to perform one function evaluation. Hence, we did not attempt to tune the linear algebra of COBYQA (although we did everything to avoid doing stupid calculations). I agree that the code can definitely be improved in terms of linear algebra (to fasten the execution when the objective function evaluations does not require a long time). Concerning the Rosenbrock function, it is indeed a pathological case, as it was designed as such. Even with @andyfaff, I agree with what you said. This is a typical example of things that can definitely be improved in the source code of COBYQA. I see you opened on issue on that matter, I will take a look at it this weekend. Again, thank you very much for the help! This is much appreciated. I will try to be more present for this matter, |
THanks for getting the suggested PR merged @ragonneau. |
It's my pleasure. Again, thank you very much for your great help, and sorry for the delay in the merge. Cheers, |
Reference issue
#19994 (comment)
What does this implement/fix?
Increase test timeout to 40 seconds