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

Fix bugs in tranquilo #449

Merged
merged 22 commits into from
Mar 28, 2023
Merged

Fix bugs in tranquilo #449

merged 22 commits into from
Mar 28, 2023

Conversation

timmens
Copy link
Member

@timmens timmens commented Mar 22, 2023

  • Make sure that centering and scaling is done via map_to_unit and map_from_unit
  • Adjust model.py to the case of sphere and cube trustregions; focus on move_model.
  • Add effective_center and effective_radius to Region
  • Add residualize option
  • Fix get_default_model_fitter
  • Use state.x instead of state.trustregion.center when appropriate

Notes for reviewer:

  • Check filter_points.py line 96-97:
    order = 2 if state.trustregion.shape == "sphere" else np.inf
    dists = np.linalg.norm(xs - state.x, axis=1, ord=order)
  • Check new_history.py line 207-213:
    out = _find_indices_in_region(xs, center=region.center, radius=region.radius)
    
    if region.shape == "sphere":
        mask = np.linalg.norm(xs[out] - region.center, axis=1) <= region.radius
        out = out[mask]
    
    return out

@codecov
Copy link

codecov bot commented Mar 22, 2023

Codecov Report

Merging #449 (6a14073) into main (3ac4dfb) will increase coverage by 0.25%.
The diff coverage is 98.98%.

❗ Current head 6a14073 differs from pull request most recent head 193523a. Consider uploading reports for the commit 193523a to get more accurate results

@@            Coverage Diff             @@
##             main     #449      +/-   ##
==========================================
+ Coverage   92.95%   93.21%   +0.25%     
==========================================
  Files         249      247       -2     
  Lines       18311    18018     -293     
==========================================
- Hits        17021    16795     -226     
+ Misses       1290     1223      -67     
Impacted Files Coverage Δ
src/estimagic/optimization/pounders_history.py 91.66% <ø> (ø)
...timagic/optimization/tranquilo/aggregate_models.py 97.67% <ø> (ø)
src/estimagic/optimization/tranquilo/tranquilo.py 97.26% <ø> (ø)
tests/visualization/test_visualize_tranquilo.py 100.00% <ø> (ø)
src/estimagic/optimization/tranquilo/models.py 95.45% <93.33%> (+1.75%) ⬆️
src/estimagic/optimization/pounders.py 93.33% <100.00%> (ø)
...imagic/optimization/tranquilo/estimate_variance.py 100.00% <100.00%> (ø)
.../estimagic/optimization/tranquilo/filter_points.py 81.13% <100.00%> (+1.29%) ⬆️
src/estimagic/optimization/tranquilo/fit_models.py 86.27% <100.00%> (+1.75%) ⬆️
src/estimagic/optimization/tranquilo/history.py 94.59% <100.00%> (ø)
... and 19 more

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

@timmens timmens requested a review from janosg March 27, 2023 12:04
@janosg
Copy link
Member

janosg commented Mar 27, 2023

About the two points you mention:

  1. I think we can decide that history searches are always based on an l2 norm and thus get rid of line 96.

  2. I think we always want to do what we currently do in the sphere case. We should check how much slower it is to just do the whole trustregion search with an l2 norm in numpy. (Background: We currently do a greedy search in numba that uses an infinity norm (i.e. no calculations, just if conditions) and stops as soon as the distance is large in any dimension).

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 a lot! Very nice.

A last general comment: We should probably replace the suffix _centered (e.g. for x, samples, ...) by _unit.

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

Very nice!

@timmens timmens merged commit 0d02ac9 into main Mar 28, 2023
@timmens timmens deleted the tranquilo-fix branch March 28, 2023 16:30
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