Skip to content

Polish new algorithms #594

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Polish new algorithms #594

wants to merge 2 commits into from

Conversation

janosg
Copy link
Member

@janosg janosg commented May 5, 2025

Polish and align a few things that slipped through the reviews for new algorithms.

  • Improve error message if iminuit is not installed
  • Set budget for PSO higher to reduce random failures
  • Test stochastic algorithms separately from deterministic algorithms in test_many_algorithms. Stochastic algorithms would be all algorithms that have a seed option. They should be tested with a fixed seed to avoid spurious failures.

Copy link

codecov bot commented May 5, 2025

Codecov Report

Attention: Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/optimagic/optimizers/iminuit_migrad.py 66.66% 2 Missing ⚠️
Files with missing lines Coverage Δ
src/optimagic/optimizers/nevergrad_optimizers.py 97.67% <100.00%> (+2.32%) ⬆️
src/optimagic/optimizers/iminuit_migrad.py 92.30% <66.66%> (-4.84%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@timmens
Copy link
Member

timmens commented May 6, 2025

I've implemented point 3 by splitting all test functions into a stochastic and deterministic one. Alternatively, we could also define algo_options conditionally inside the test function depending on whether the algorithm is stochastic or not.

@janosg
Copy link
Member Author

janosg commented May 6, 2025

I've implemented point 3 by splitting all test functions into a stochastic and deterministic one. Alternatively, we could also define algo_options conditionally inside the test function depending on whether the algorithm is stochastic or not.

Thinking about that: We could just have called every algorithm with a seed. Optimagic would discard it automatically where it doesn't apply 🤦

On the other hand: Separating the stochastic algorithms will allow us to use something like flaky to re-run stochastic tests which is something we definitely don't want to do for deterministic algorithms.

So I would say we keep the separation.

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