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.

Sign up for GitHub

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

Jump to bottom

Implement the new interface for objective functions and derivatives #512

Merged
merged 32 commits into from
Aug 9, 2024
Merged

Implement the new interface for objective functions and derivatives #512

merged 32 commits into from
Aug 9, 2024

Conversation

janosg
Copy link
Member

@janosg janosg commented Aug 1, 2024

The changes for the objective function are described here

The changes for derivatives are described here

The jax integration is postponed to a later PR as it does not entail any breaking changes or deprecations.

@review-notebook-app Review Notebook App
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@janosg janosg changed the base branch from main to 0.5.0 August 1, 2024 16:30
@codecov Codecov
Copy link

codecov bot commented Aug 2, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 96.00000% with 25 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/estimagic/estimate_ml.py 69.56% 7 Missing ⚠️
src/optimagic/mark.py 88.09% 5 Missing ⚠️
...imagic/optimization/create_optimization_problem.py 91.37% 5 Missing ⚠️
src/optimagic/optimization/fun_value.py 97.82% 3 Missing ⚠️
src/optimagic/optimization/multistart.py 80.00% 2 Missing ⚠️
src/estimagic/examples/logit.py 92.30% 1 Missing ⚠️
src/optimagic/optimization/process_results.py 91.66% 1 Missing ⚠️
src/optimagic/visualization/slice_plot.py 93.75% 1 Missing ⚠️
Files with missing lines Coverage Δ
src/estimagic/estimate_msm.py 89.47% <100.00%> (ø)
src/optimagic/__init__.py 100.00% <100.00%> (ø)
...c/optimagic/benchmarking/get_benchmark_problems.py 93.87% <100.00%> (ø)
src/optimagic/benchmarking/more_wild.py 100.00% <100.00%> (ø)
...ptimagic/benchmarking/process_benchmark_results.py 89.33% <ø> (ø)
src/optimagic/decorators.py 90.08% <100.00%> (ø)
src/optimagic/deprecations.py 100.00% <100.00%> (ø)
src/optimagic/differentiation/derivatives.py 98.14% <100.00%> (ø)
src/optimagic/examples/criterion_functions.py 100.00% <100.00%> (ø)
src/optimagic/optimization/error_penalty.py 87.69% <100.00%> (ø)
... and 16 more

janosg and others added 24 commits August 2, 2024 16:24
@janosg
Fix merge conflicts.
375ed28
@janosg
Add internal_value method to subclasses of FunctionValue.
eada6b8
@janosg
Renaming.
0801733
@janosg
Combine the different enforce decorators into one.
3c33c22
@janosg
Do the core change.
69303bd
@janosg
Fix merge conflicts.
6205334
@janosg
Remove dead code.
97269e7
@janosg
Combine ProblemType and SolverType to AggregationLevel.
6bdf854
@janosg
Cleanup.
71bb339
@janosg
Cleanup.
3836931
@janosg
Fix merge conflicts.
114eafd
@janosg
Clean up criterion_functions.py.
a07375f
@timmens
Fix deprecation warnings stemming from derivatives (#516)
7835c66
@janosg
Fix future warnings.
c6f1bde
@janosg
Add full pytree handling to criterion_functions.py.
c6205b0
@janosg
Fix warnings in estimate_ml.
faae4fa
@janosg
Fix remaining warnings.
7b5b514
@janosg
Implement new interface for derivatives.
6020863
@janosg
Add tests for new function formats.
09a173a
@janosg
Add tests for future warnings.
e1ea722
@janosg
Remove problem_type from OptimizationProblem.
a352527
@janosg
Add how-to guide for objective functions.
ad94056
@janosg
Fix.
19ce4a8
@janosg
Write how-to guide for derivatives.
2cd1aa7
timmens
timmens reviewed Aug 9, 2024
View reviewed changes
Copy link
Member

@timmens timmens left a comment

Choose a reason for hiding this comment

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

Very nice PR, this is a big leap towards the new release 🎉

I only have a few clarifying questions. Once these are discussed, I will approve.

src/estimagic/estimate_msm.py Show resolved Hide resolved
src/optimagic/deprecations.py Show resolved Hide resolved
src/optimagic/mark.py Outdated Show resolved Hide resolved
src/optimagic/optimization/create_optimization_problem.py Outdated Show resolved Hide resolved
src/optimagic/shared/process_user_function.py Outdated Show resolved Hide resolved
src/optimagic/optimization/multistart.py Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Why can we not use the .internal_value(solver_type) strategy here?

Copy link
Member Author

Choose a reason for hiding this comment

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

We are not simply evaluating a decorated version of the user provided fun but internal_criterion_and_derivative with the task criterion. So we don't get a SpecificFunctionValue object here. But I could get rid of the manual aggregation in a different way, by pretending we are using internal_criterion_and_derivative for a scalar optimizer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Still couldn't delete _aggregate_func_output_to_value but should be possible when we have the new internal algorithm interface (which will have better result objects).

tests/estimagic/test_estimate_ml.py Outdated Show resolved Hide resolved
tests/optimagic/optimization/test_error_penalty.py Show resolved Hide resolved
tests/optimagic/shared/test_process_user_functions.py Outdated Show resolved Hide resolved
tests/optimagic/test_mark.py Outdated Show resolved Hide resolved
timmens
timmens reviewed Aug 9, 2024
View reviewed changes
docs/source/how_to/how_to_criterion_function.ipynb Show resolved Hide resolved
docs/source/how_to/how_to_criterion_function.ipynb Show resolved Hide resolved
docs/source/how_to/how_to_derivatives.ipynb Show resolved Hide resolved
docs/source/how_to/how_to_derivatives.ipynb
@@ -0,0 +1,276 @@
{
Copy link
Member

@timmens timmens Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Line #6.        fun=sphere,

Do we need to pass fun if we pass fun_and_jac?


Reply via ReviewNB

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently yes but I created an issue to remove this limitation.

docs/source/how_to/how_to_derivatives.ipynb Show resolved Hide resolved
timmens
timmens approved these changes Aug 9, 2024
View reviewed changes
Copy link
Member

@timmens timmens left a comment

Choose a reason for hiding this comment

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

Looks good, only one comment about a comment.

src/optimagic/shared/process_user_function.py Outdated Show resolved Hide resolved
@janosg janosg merged commit 735dfe1 into 0.5.0 Aug 9, 2024
16 checks passed
@janosg janosg deleted the criterion-interface branch August 9, 2024 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timmens timmens timmens approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@janosg @timmens