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

Add wrappers for pyensmallen #566

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

Conversation

gauravmanmode
Copy link
Contributor

@gauravmanmode gauravmanmode commented Mar 13, 2025

This PR addresses issue #551 .
added lbfgs barebone

Copy link

codecov bot commented Mar 13, 2025

Codecov Report

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

Files with missing lines Patch % Lines
src/optimagic/config.py 60.00% 2 Missing ⚠️
Files with missing lines Coverage Δ
src/optimagic/algorithms.py 85.73% <100.00%> (+0.04%) ⬆️
src/optimagic/optimizers/pyensmallen_optimizers.py 100.00% <100.00%> (ø)
src/optimagic/config.py 70.14% <60.00%> (-0.82%) ⬇️
🚀 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.

Hi @gauravmanmode, thanks for your PR. I made a few comments but this is definitely going into the right direction.

Comment on lines +84 to +85
x=raw, # only best x is available
fun=problem.fun(raw), # best f(x) value is not available
Copy link
Member

Choose a reason for hiding this comment

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

Is there a chance to get more infos (like an iteration counter, etc.) out of the pyensmallen results object? It would also be good to get the optimal function value out of pyensmallen instead of doing an extra function evaluation on top here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

currently, the python wrapper for ensmallen is very minimalistic. the result object is just the best x value. it is possible to get a more detailed report by passing a Report() callback function but it is not implemented in the wrapper yet.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. This is something we'll need to address before we can merge.

@janosg
Copy link
Member

janosg commented Mar 15, 2025

Hi @gauravmanmode, @timmens and I quickly looked into the failures on GitHub actions and found the following:

  • Since pyensmallen will be an optional dependency, you have to put all imports into if statements. See fides as an example.
  • Most of the failures are expected due to known limitations of the pyensmallen wheels (see e.g. here. Here the only thing we can do for now is skip the tests on non-linux systems and for Python >= 3.13; For all tests you are writing you can use pytests skipif decorator to enable this.
  • On some systems the failure is already at the environment creation. @timmens will post a workaround for this.

@timmens
Copy link
Member

timmens commented Mar 15, 2025

Hey!

After a quick discussion with Janos we came to the conclusion that it is easiest if you solve the environment problem by

  1. Removing the pyensmallen library from the environment.yml file (since it cannot be installed on Windows this would be problematic in any case). After running the pre-commit hooks this should also remove the library from the testing environments.
  2. In the GitHub Actions specification file (main.yml) you have to split the "run pytest" step into two steps: one should run only on Python 3.13 (adjust the name accordingly) and the other one should run on Python < 3.13. For the latter one, install pyensmallen via pip after activating the optimagic environment and before running pytest. To skip a step conditional on the Python version, check out the if: ... line in the "Upload coverage report" step.

If you have any questions or need help, feel free to ask; @janosg will be happy to help!

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@gauravmanmode gauravmanmode marked this pull request as ready for review March 18, 2025 18:32
@gauravmanmode
Copy link
Contributor Author

gauravmanmode commented Mar 18, 2025

I have done the above said changes.

This is the alignment of tuning parameters for:

ensmallen_lbfgs

Old Name Proposed Name from optimizer in optimagic
numBasis limited_memory_storage_length scipy
maxIterations stopping_maxiter scipy
armijoConstant armijo_constant needs review
wolfe wolfe_condition needs review
minGradientNorm convergence_gtol_abs scipy
factr convergence_ftol_rel scipy
maxLineSearchTrials max_line_search_trials needs review
minStep min_step_for_line_search needs review
maxStep max_step_for_line_search scipy

I have attached a jupyter notebook which i used for testing and exploring the library.
link

@janosg
Copy link
Member

janosg commented Mar 19, 2025

I have done the above said changes.

This is the alignment of tuning parameters for:

ensmallen_lbfgs

Old Name Proposed Name from optimizer in optimagic
numBasis limited_memory_storage_length scipy
maxIterations stopping_maxiter scipy
armijoConstant armijo_constant needs review
wolfe wolfe_condition needs review
minGradientNorm convergence_gtol_abs scipy
factr convergence_ftol_rel scipy
maxLineSearchTrials max_line_search_trials needs review
minStep min_step_for_line_search needs review
maxStep max_step_for_line_search scipy
I have attached a jupyter notebook which i used for testing and exploring the library. link

Hi @gauravmanmode, thanks for the table. The names are really good. The only thing I would change is max_line_search_trials -> max_line_search_steps because wa already use that name in scipy_lbfgsb. (Unless I'm misunderstanding something and the options are conceptually different).

@gauravmanmode
Copy link
Contributor Author

hi,
both max_line_search_steps and max_line_search_trials are the same.
also, i should be able to get a more detailed OptimizationReport from pyensmallen in a few days.

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