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

Improve argument processing of tranquilo #446

Merged
merged 18 commits into from
Mar 21, 2023
Merged

Improve argument processing of tranquilo #446

merged 18 commits into from
Mar 21, 2023

Conversation

janosg
Copy link
Member

@janosg janosg commented Mar 17, 2023

Problems

  1. The default configuration of tranquilo is quite complex. This is easiest shown with an example: The default target_sample_size depends on the model_type; The default model_type depends on the functype. But the model_type can also be overwritten by the user. If so, we need to check that the user specified a valid model type before we can create the default target_sample_size. I.e. everything is entangled and the order of processing stuff has to be chosen very carefully.
  2. The default values are not collected in one place.
  3. The namespace of the main tranquilo function is cluttered with stuff that was only needed to initialize the main components; It is hard to tell which variables will get used later on and which can be easily changed.

Solution

  • All default values should be defined in options.py; If default values depend on other arguments, we implement getter_functions, e.g. get_default_radius_options(x)
  • Nothing else is in options.py; In particular, all the logic related to combining default options and user options is in process_arguments.py.
  • The outer tranquilo function only calls process_arguments and then calls _internal_tranquilo with the processed arguments. This removes all namespace cluttering. A small trick to avoid long lists of arguments, the outer tranquilo functions only uses *args, **kwargs as arguments and functools.wraps(process_arguments) to get the right signature and docstring.

Alternatives

We could use dags to figure out the order of everything, but we currently do not have dags as a dependency and we don't need it anywhere else. In any case, the current PR gets us closer to a situation where we could use dags.

To-Do

  • Move the actual logic from tranquilo into _internal_tranquilo
  • Rename _new_tranquilo to _tranquilo
  • Run all tests and a benchmark
  • Make RadiusOptions.initial_radius a mandatory argument without default value
  • Make all ConvOptions mandatory
  • Make all StopOptions mandatory
  • Write some tests for the functions in process_arguments and options.py
  • Put all component default options into options.py. Example:
    • Define a class SubsolverOptions that contains everything the default_options dict in solve_subproblem contains
    • Adjust get_component sucht that the user_options can be NamedTuples on which we call _asdict
    • Instantiate the new class in solve_subproblem instead of defining a dict there
  • Improve error message in default aggregator and move the entire compatibility check from aggregate_models into get_default_aggregator if possible.
  • make default model fitter dependent on model type and sample size instead of functype
  • Improve error handling and type checking inside process_arguments

Discuss

  • Check up on differences in benchmarks introduced by commit 9d68fab
  • Double-check if we want to divide AcceptanceOptions into individual options in acceptance_decision.py.

@codecov
Copy link

codecov bot commented Mar 17, 2023

Codecov Report

Merging #446 (bf07d2a) into main (91ffcb8) will increase coverage by 0.05%.
The diff coverage is 99.69%.

@@            Coverage Diff             @@
##             main     #446      +/-   ##
==========================================
+ Coverage   92.99%   93.05%   +0.05%     
==========================================
  Files         247      250       +3     
  Lines       18482    18597     +115     
==========================================
+ Hits        17188    17305     +117     
+ Misses       1294     1292       -2     
Impacted Files Coverage Δ
...sts/optimization/subsolvers/test_gqtpar_lambdas.py 100.00% <ø> (ø)
...optimization/tranquilo/test_acceptance_decision.py 100.00% <ø> (ø)
tests/optimization/tranquilo/test_rho_noise.py 100.00% <ø> (ø)
src/estimagic/optimization/tranquilo/options.py 99.18% <98.92%> (-0.82%) ⬇️
...agic/optimization/tranquilo/acceptance_decision.py 96.87% <100.00%> (ø)
...c/optimization/tranquilo/acceptance_sample_size.py 95.45% <100.00%> (ø)
...timagic/optimization/tranquilo/aggregate_models.py 97.67% <100.00%> (+10.17%) ⬆️
...imagic/optimization/tranquilo/estimate_variance.py 100.00% <100.00%> (ø)
.../estimagic/optimization/tranquilo/filter_points.py 79.83% <100.00%> (+0.16%) ⬆️
src/estimagic/optimization/tranquilo/fit_models.py 84.51% <100.00%> (-1.85%) ⬇️
... and 10 more

... and 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member Author

@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. Three minor points. After they are fixed I'll run another benchmark and then we can merge.

src/estimagic/optimization/tranquilo/get_component.py Outdated Show resolved Hide resolved
src/estimagic/optimization/tranquilo/options.py Outdated Show resolved Hide resolved
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.

Great PR. This is an important step forward to disentangle the actual algorithm development of tranquilo and its options management.

@timmens timmens merged commit bea7abb into main Mar 21, 2023
@timmens timmens deleted the option-processing branch March 21, 2023 14:12
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