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

Profile: improve profiling code #1447

Open
wants to merge 36 commits into
base: develop
Choose a base branch
from
Open

Profile: improve profiling code #1447

wants to merge 36 commits into from

Conversation

Doresic
Copy link
Contributor

@Doresic Doresic commented Aug 14, 2024

I implemented improvements and changes to the profiling algorithm and visualization. The following was tested on 5 models (Boehm, Rosenbrock, Randall, Yuhong, Wuyan) and has seen improvements in robustness and stability of profiles.

Major changes:

  • Changed the default profiling next_guess_method from adaptive_step_regression to adaptive_step_order_1. The former had a problem with poor extrapolation -- polynomials of high degree are known to diverge outside of the fitting region. In the testing I've done, the latter was more robust and faster.
  • The adaptive method chooses a heuristic next_obj_target and changes the step length until the proposed objective function value is stepped over. Until now, the adaptive method considered only a high_next_obj_target value that is higher than the current objective function value. This caused it to make large steps if the profiler was in a region of decreasing objective function values. Thus, I added the low_next_obj_target as well. If any of the two is reached, the corresponding step is returned.
  • In many cases, the local optimization in walk_along_profile failed. In most cases, this was due to a poor initial proposal of next_x from which the optimization was started. In cases where this happened, the only fail-safe was to try to resample the next_x uniformly. This made the profile very discontinuous as it jumped randomly through the parameter space. Thus, in case of optimization failure before resorting to resampling of next_x I made the algorithm re-try the next_guess generation, but this time with different max_step_size and min_step_size hyperparameters. First, the next_guess function is called with an increasingly smaller max_step_size to try to stay closer to the last point which had a successful optimization. This is done until optimization is successful or max_step_size is reduced below min_step_size. Then, the idea is to try to jump over the "region of poor simulation/optimization" by iteratively calling next_guess with an increasingly bigger min_step_size. If these two do not succeed the next_x is randomly re-sampled as a last resort.
  • I changed some things about the profiles() visualization:
    • The visualization now plots dots to show the density of the profile. Simple, but nice to have.
    • Added a plot_objective_values option. This makes the plotting routine plot the objective function value across the parameter values instead of the default likelihood ratio. It was interesting to inspect here and there, so I left it there as I think it might be nice for others.
    • Added y-ticks to all subplots, as the scales of different subplots can be drastically different.
    • Added a quality_colors option. As the profile now has adaptive max_step_size and min_step_size values, and sometimes even re-samples the points, it's nice to know when what happens. Thus, red points indicate a step with a reduced max_step_size, blue points indicate a step with an increased min_step_size and green points indicate that the next guess x_next was resampled. If none of these happened, the color is black. These colors are saved throughout profiling in ProfilerResult.x_color_path.

Minor changes:

  • Added constant logging.INFO information about the progress of the profiling. There was nothing of this sort there so it was impossible to know if the profiling entered some buggy infinite loop.
  • Changed the form of the next_obj_target to depend on the objective function distance to the last profile point (last_delta_fval) rather than the distance to the global optimum objective function value (delta_obj_value). The latter made it make larger steps in low-likelihood regions. However, I'm not sure whether it is desirable to that degree.
  • Added a trust region of extrapolation in all parameters other than the one that's being profiled. This restricts the extrapolations from making too large steps.
  • Had to increase step checks in the profile tests -- The adapted algorithm makes more steps in regions of decreasing objective function values.

Fixed some bugs:

  • If the profiling was started at a lower parameter boundary, the order one extrapolation would explode on the first step to the right as the x difference was 0 -> division by 0.
  • There was a bug with stopping criteria when reaching the left boundary. The step returned would be on the other side, and the profiling would bounce off the edge, and continue in the other direction when it shouldn't -- ending up in an infinite loop without exit condition. Absolute values at the right spots fixed it.

Implement two targets: higher and lower. Choose which one to go to depending on first guess.
Different calculation of next_obj_target

TODO: change 1.5 to magic factor
Implemented adaptive max and min steps in profiling. If the optimization during profiling fails (results in inf value), the algorithm will first try to iteratively decrease `max_step_size` to be closer to the last point that had a successful optimization. If that doesn't work (if we reduce max_step_size below min_step_size), then max_step_size is set back to the default and we try to increase min_step_size to "jump over" the problematic area.

Resampling random points and start from those is only the last resort and will be done if these two do not work. The idea is that we want to stay as close as we can to the last profiling point.

TODO: Put the adaptive reduction/increase of max_step_size/min_step_size into options
- BUGFIX: absolute value in objective targets at last_delta_fval
- BUGFIX: extrapolation explosions if we start at boundary
- Feature: Trust region on extrapolation
- Added y ticks back into the plot, sometimes the range is completely different.
- Added points to the plotting of profiles (in case of one result and one profile list id)
- Added color change to plotting of profiles (in case of one result and one profile list id)
- LOGGING: added logging.INFO with informations of steps made and successful optimizations.
@Doresic Doresic marked this pull request as draft August 14, 2024 13:31
@codecov-commenter
Copy link

codecov-commenter commented Aug 14, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 70.92199% with 41 lines in your changes missing coverage. Please review.

Project coverage is 82.71%. Comparing base (2eee081) to head (8ef3b51).
Report is 3 commits behind head on develop.

Files with missing lines Patch % Lines
pypesto/profile/walk_along_profile.py 48.93% 24 Missing ⚠️
pypesto/visualize/profiles.py 74.35% 10 Missing ⚠️
pypesto/profile/profile_next_guess.py 87.23% 6 Missing ⚠️
pypesto/profile/options.py 66.66% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1447      +/-   ##
===========================================
- Coverage    82.88%   82.71%   -0.18%     
===========================================
  Files          163      163              
  Lines        13786    13894     +108     
===========================================
+ Hits         11427    11492      +65     
- Misses        2359     2402      +43     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines 115 to 116
if self.magic_factor_obj_value < 0 or self.magic_factor_obj_value >= 1:
raise ValueError("magic_factor_obj_value must be >= 0 and < 1.")
if self.magic_factor_obj_value < 1:
raise ValueError("magic_factor_obj_value must be > 1.")
Copy link
Contributor Author

@Doresic Doresic Sep 25, 2024

Choose a reason for hiding this comment

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

The magic factor now should be larger than 1 rather than between 0 and 1. The next_obj_target now depends on the objective function value difference of the last two profile points, rather than the difference of the last profile point to the global minimum. The former is in general much smaller than the latter, so we want to increase it rather than decrease it using a factor.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a better name for this? magic_factor sounds fun but is rather uninformative. Also: if it stays like that, documentation in line 35-37 needs to be adjusted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True...
It is kinda the scaling factor of the adaptive target value.
Changed to adaptive_target_scaling_factor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Take a look at the documentation of it in ProfileOptions. Not sure if informative enough.

Comment on lines -212 to +247
step_length = problem.lb_full[par_index] - x[par_index]
return x + step_length * delta_x_dir
step_length = abs(problem.lb_full[par_index] - x[par_index])
return clip_to_bounds(x + step_length * delta_x_dir)

if par_direction == 1 and (min_delta_x > problem.ub_full[par_index]):
step_length = problem.ub_full[par_index] - x[par_index]
return x + step_length * delta_x_dir
step_length = abs(problem.ub_full[par_index] - x[par_index])
return clip_to_bounds(x + step_length * delta_x_dir)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is where the bug with the exit condition was. Absolute value fixes it.

Comment on lines 541 to 556
# Determine the direction of the step
if next_obj >= current_obj:
next_obj_target = high_next_obj_target
if next_obj >= high_next_obj_target:
direction = "decrease"
decreasing_to_high_target = True
else:
direction = "increase"

elif next_obj < current_obj:
next_obj_target = low_next_obj_target
if next_obj <= low_next_obj_target:
direction = "decrease"
decreasing_to_low_target = True
else:
direction = "increase"
Copy link
Contributor Author

@Doresic Doresic Sep 25, 2024

Choose a reason for hiding this comment

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

If the initial guess is between the lower and higher target, we increase step size till we hit any of them. (exit conditions below)

If the initial guess is below lower target or above higher target, we decrease the step size till we cross the lower or higher target, respectively.

@Doresic Doresic marked this pull request as ready for review September 25, 2024 15:03
@Doresic
Copy link
Contributor Author

Doresic commented Sep 26, 2024

Randall_example_profile
Example showcasing the quality coloring (Randall model)

@Doresic
Copy link
Contributor Author

Doresic commented Sep 30, 2024

example_randall_obj_fun
This is the same profile, but the plotted value is the objective function rather than the log-posterior ratio (plot_objective_values=True).
Here it's easy to see how the steps at which next_x was resampled (green points) correspond to jumps in the objective function.

As a reference, this is the objective function value profile using the old code:
Reference_old_profile

Copy link
Collaborator

@PaulJonasJost PaulJonasJost left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me. Would try to decrease intendation wherever possible for easier understanding

Comment on lines 115 to 116
if self.magic_factor_obj_value < 0 or self.magic_factor_obj_value >= 1:
raise ValueError("magic_factor_obj_value must be >= 0 and < 1.")
if self.magic_factor_obj_value < 1:
raise ValueError("magic_factor_obj_value must be > 1.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a better name for this? magic_factor sounds fun but is rather uninformative. Also: if it stays like that, documentation in line 35-37 needs to be adjusted.

pypesto/profile/profile.py Show resolved Hide resolved
pypesto/profile/profile.py Show resolved Hide resolved
pypesto/profile/profile_next_guess.py Show resolved Hide resolved
pypesto/profile/profile_next_guess.py Show resolved Hide resolved
pypesto/profile/walk_along_profile.py Show resolved Hide resolved
pypesto/profile/walk_along_profile.py Outdated Show resolved Hide resolved
@Doresic Doresic requested a review from vwiela as a code owner October 10, 2024 12:20
Copy link
Contributor

@m-philipps m-philipps 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 improvement! I checked the visualize file.

pypesto/visualize/profiles.py Outdated Show resolved Hide resolved
pypesto/visualize/profiles.py Show resolved Hide resolved
pypesto/visualize/profiles.py Outdated Show resolved Hide resolved
Co-authored-by: Maren Philipps <[email protected]>
Copy link
Contributor

@vwiela vwiela 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 to me, thanks for improving.

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.

6 participants