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

added initial wrapper for migrad #568

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

Conversation

spline2hg
Copy link
Contributor

@spline2hg spline2hg commented Mar 14, 2025

The basic functionality has been tested with the following Python code:

import numpy as np
import optimagic as om

def cost_function_with_two_minima(x):
    return x**4 - x**2 + 1


res = om.minimize(
    fun=cost_function_with_two_minima,
    params=0.1,
    algorithm="iminuit_migrad",
    bounds=om.Bounds(
        lower=0,
        upper=0.6
    )
)

print(res)

Copy link

codecov bot commented Mar 17, 2025

Codecov Report

Attention: Patch coverage is 94.91525% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/optimagic/config.py 60.00% 2 Missing ⚠️
src/optimagic/optimizers/iminuit_migrad.py 97.14% 1 Missing ⚠️
Files with missing lines Coverage Δ
src/optimagic/algorithms.py 85.77% <100.00%> (+0.08%) ⬆️
src/optimagic/optimization/algo_options.py 100.00% <100.00%> (ø)
src/optimagic/optimizers/iminuit_migrad.py 97.14% <97.14%> (ø)
src/optimagic/config.py 70.14% <60.00%> (-2.44%) ⬇️

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@janosg janosg left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I made a few comments in the code.

In addition:

  • We need more tests, for example for the bounds conversion
  • We need documentation

It looks surprising that the only tuning parameter of the algorithm is stopping_maxfun. Are there no other convergence criteria or tuning parameters?

@spline2hg
Copy link
Contributor Author

hi @janosg,

thanks for the review! i will go through all the suggestions and make the necessary changes.

regarding stopping_maxfun being the only parameter, i had the same doubt. i found the following things

  1. uses edm as convergence criterion:

  2. iminuit.minuit and migrad parameters:

    • the only parameters that seem compatible with algo_options in iminuit.minuit and migrad are ncall (max function calls) and iterate (migrad iterations).
    • i hadn’t set the iterate parameter because STOPPING_MAXITER has a value of 1,000,000, and the docs suggest starting with iter >= 5. i thought restarting optimization STOPPING_MAXITER times if it doesn’t converge wouldn’t be right, but i have set it now. let me know if you have any thoughts.
    • iminuit.minuit documentation
    • migrad documentation
  3. internal interface:

    • the actual low-level process within iminuit, which includes parameters like tolerance and precision, is within the _robust_low_level_fit function. these parameters are not directly exposed to users via the iminuit.minuit interface.
    • iminuit github source

i have tried to verify these points, but i might have missed something. happy to make further changes if needed!

@janosg
Copy link
Member

janosg commented Mar 23, 2025

Hi @spline2hg,

Thanks for the detailed reply.

Ok, I am convinced that you did not miss any tuning parameters!

Note that iter is very different from what is usually set via stopping_maxiter. It is not counting actual optimizer iterations, which are usually in the thousands but actually running the optimizer multiple times. We want to support this option but a better name would be n_restarts and the default value should be 1.

In the very long run we might look into the low-level interface but for now it is ok to just support two options.

Of course, it's important to document them well and also to document why it is not possible to set additional convergence criteria.

@spline2hg
Copy link
Contributor Author

@janosg, do I need to add iminuit to the rtd_environment.yml file for the documentation build to pass? If not, could you help me identify the error?

@janosg
Copy link
Member

janosg commented Apr 1, 2025

It was a temporary readthedocs problem.

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