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

Implementation of Iterative ProductMode() #184

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

LukasBarner
Copy link

As discussed on discourse, an iterative solution strategy for ProductMode() would be nice.
I have given this a first (and probably naive) shot.
Changes include:

  • added dependency Printf to format output strings in the iterative log
  • added a vector of regularization parameters to ProductMode to be iteratively decreased for each solve
  • added a vector of constraint references pointing to the complementarity constraints
  • added an iterative solution procedure in the optimize!() function, including logging.
    I would have preferred to do the iterative procedure in place (i.e. without passing a copy for each iteration). The commented code would implement this. However, copying seems more appropriate in this case, as in place modification does not work.
    Changes are small, so no old functionality should break.

I would be happy about a brief discussion, especially in case anyone sees a more elegant approach.
Warnings/errors are not yet implemented, also not thoroughly tested on my end. This is more to test the waters and not a final approach :D
It's my first PR, so I would be happy about some feedback!

@LukasBarner
Copy link
Author

I edited a bit of the code to make it more concise and a little more performant.

For practical usage, it is important to set the right solver attributes before running the model. I guess this can hardly be done inside BilevelJuMP ex-ante, as code should work independent of the solver.
For example, in Ipopt the following settings should be made:

set_optimizer_attribute(model, "warm_start_init_point", "yes")
set_optimizer_attribute(model, "warm_start_bound_push", 1e-12)
set_optimizer_attribute(model, "warm_start_bound_frac", 1e-12)
set_optimizer_attribute(model, "warm_start_slack_bound_frac", 1e-12)
set_optimizer_attribute(model, "warm_start_slack_bound_push", 1e-12)
set_optimizer_attribute(model, "warm_start_mult_bound_push", 1e-12)

Additionally, not printing the solver log gives a higher level overview.
set_optimizer_attribute(model, "print_level", 0)

In the future, some of the warmstart related settings may be included automatically for popular solvers, but I think leaving this to the user should be sufficient for now :D

… lost) and separate solver settings in iterative mode
…hout using copy_to each iteration, except if this is necessary from solver side.
@LukasBarner
Copy link
Author

Hey @joaquimg,
I have now also implemented a default solution strategy without iteratively using copy_to (this seems to be considerably better from a performance point of view).
I have tested my code for bigger problems on a cluster computer, and it allowed me to solve problems that were not solvable with sufficiently low regularization via the one-shot strategy.
There are still a few points that I'm not really happy with (for example the comp_idxs_in_solver (returned by get_solver_comp_idxs), but after some testing I believe the code works as intended and doesn't break anything existing.
In case you are testing with Ipopt, there are still a few problems though:

@LukasBarner LukasBarner marked this pull request as draft September 9, 2022 16:31
@LukasBarner LukasBarner marked this pull request as ready for review October 3, 2022 16:14
@LukasBarner
Copy link
Author

Think this should be good to go for now.
@joaquimg, could you give some feedback?

@joaquimg
Copy link
Owner

joaquimg commented Oct 3, 2022

Will look into it!

Copy link
Owner

@joaquimg joaquimg left a comment

Choose a reason for hiding this comment

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

I like where it is going.
Can you fix the JuMP style?
I will make changes following that

src/jump.jl Outdated

function _iterative_optimize_copy!(single_blm, solver, mode::ProductMode{T}, model::BilevelModel, t0) where T
for (iter,eps) in enumerate(mode.IterativeEpsilon)
SetIterPrimalStarts!(single_blm, solver, model.sblm_to_solver)
Copy link
Owner

Choose a reason for hiding this comment

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

in many functions and variable names this PR does nof follow the JuMP style guide: https://jump.dev/JuMP.jl/stable/developers/style/

Copy link
Author

Choose a reason for hiding this comment

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

I'm still fairly inexperienced, so please excuse the question. Is this mostly about naming functions/variables like SetIterPrimalStarts! or IterEps?
Or do I need to consider anything else?

Copy link
Author

Choose a reason for hiding this comment

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

OK, I have now taken care of function and variable names as well as code formatting.

@LukasBarner
Copy link
Author

Now includes tests and a mutable log as well. Note that I had to update Ipopt compat (because it did not have methods to alter the rhs prior to jump-dev/Ipopt.jl#336).

@joaquimg
Copy link
Owner

joaquimg commented Oct 6, 2022

Formatter was added to master, so you can just run that and it will fix part of the style.

@LukasBarner
Copy link
Author

can you check if this is better?

@LukasBarner
Copy link
Author

Just realized that the following line is problematic if there exist different types of complements (greater/less) within the same model:
https://github.com/LukasBarner/BilevelJuMP.jl/blob/88561d99bb8045feef1926bf8f86b9733d7acdb7/src/jump.jl#L1060
To keep everything as easy as possible, I flipped the second complementarity constraint for conic problems.
In case this is not a solution you prefer, I could also do something else (like a conditional, or something more type-stable).

Further, the previous attempt did not work correctly with slacks, which is also fixed now.

@LukasBarner
Copy link
Author

@joaquimg is there anything I can do/fix atm?

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