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

A few minor cleanups #19

Closed
wants to merge 2 commits into from
Closed

A few minor cleanups #19

wants to merge 2 commits into from

Conversation

rdeits
Copy link
Contributor

@rdeits rdeits commented Dec 18, 2017

Thanks for making this available! I'm playing around with connecting PATH to https://github.com/rdeits/ConditionalJuMP.jl and noticed some opportunities to clean up the code a bit. This PR does two things:

  1. get rid of the redundant solveLCP methods without var_name and con_name arguments and replace them with default values. Default argument values in Julia do exactly what you were doing by hand here, so there's no reason to write out the other methods.
  2. get rid of the global user_f and user_j functions. You can call cfunction on a closure with no issues, so we can create a custom cfunction for the user's particular callback.

Currently I haven't observed (2) making any performance difference, but I'm planning to build further on that change to solve LCPs (without calling ForwardDiff.jacobian at every iteration).

@coveralls
Copy link

coveralls commented Dec 18, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling e29f4da on rdeits:cleanup into 7e4928a on chkwon:master.

@rdeits
Copy link
Contributor Author

rdeits commented Dec 19, 2017

Oops, point (2) was not entirely true. I'll find a more elegant solution.

@chkwon
Copy link
Owner

chkwon commented Dec 22, 2017

Thanks again for your PR. Your project ConditionalJuMP.jl looks quite interesting. For complementarity constraints, I tried to do some work in https://github.com/chkwon/Complementarity.jl

See https://github.com/chkwon/Complementarity.jl/blob/master/MPEC.md

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.

3 participants