-
Notifications
You must be signed in to change notification settings - Fork 40
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
Embedding Formulation #73
Conversation
Codecov Report
@@ Coverage Diff @@
## master #73 +/- ##
========================================
+ Coverage 78.21% 79% +0.79%
========================================
Files 27 28 +1
Lines 5696 5898 +202
========================================
+ Hits 4455 4660 +205
+ Misses 1241 1238 -3
Continue to review full report at Codecov.
|
embedding::Bool | ||
embedding_encode::Any # Encoding method used for embedding | ||
embedding_ibs::Bool # Enable independent branching scheme | ||
embedding_link::Bool # Linking constraints between x and α, type 1 usse hierarchical and type 2 with big-m |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this embedding_link for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for an experiment to try out the alternative bounding constraints (y1b0+y1b1<=x<=y1b1+y2b2) when you have log # of binary variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this experiment successful - did it improve computation time and bounds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/embedding.jl
Outdated
return v | ||
end | ||
|
||
# # Version 1 implementation # |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clean up the commented part - or may be remove it it we are not going to use it any more. Or if you dont want to remove it - make it as a function and add a comment saying old version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is taken care of in the most recent commit.
src/embedding.jl
Outdated
@@ -0,0 +1,253 @@ | |||
# Make sure two things :: correct setup & compatible mapping function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment for each function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is taken care of in the most recent commit.
src/multi.jl
Outdated
@@ -170,8 +179,7 @@ function populate_convhull_extreme_values(m::PODNonlinearModel, discretization:: | |||
end | |||
|
|||
""" | |||
TODO: docstring | |||
method for general multilinear term | |||
Extreme value for gneral multilinear terms Efficient implementation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
general
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is taken care of in the most recent commit.
src/multi.jl
Outdated
@@ -197,7 +205,7 @@ function populate_convhull_extreme_values(m::PODNonlinearModel, discretization:: | |||
end | |||
|
|||
""" | |||
Less memeory & time efficient, but a easier implementation | |||
Less memeory & time efficiency, but a easier implementation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
memory*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is taken care of in the most recent commit.
src/multi.jl
Outdated
@@ -197,7 +205,7 @@ function populate_convhull_extreme_values(m::PODNonlinearModel, discretization:: | |||
end | |||
|
|||
""" | |||
Less memeory & time efficient, but a easier implementation | |||
Less memeory & time efficiency, but a easier implementation | |||
""" | |||
function _populate_convhull_extreme_values(discretization::Dict, ml_indices::Any, λ::Dict, extreme_point_cnt::Int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make function names consistent why is there an underscore before the function name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function _populate_convhull_extreme_values() is not actively used. But it serves the purpose of verifying the populate_convhull_extreme_values() just in case the multi-dimensional scheme in Julia is different.
src/multi.jl
Outdated
@@ -347,18 +381,16 @@ function valid_inequalities(m::PODNonlinearModel, discretization::Dict, λ::Dict | |||
return | |||
end | |||
|
|||
# Minimum Formulation with Boundary Cuts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we still need this - let me discuss with harsha and russell and get back to you on this - for now have it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. This is good for testing purposes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this function with its solver options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. This is addressed in the most recent commit. Just a note, this option is removed in a later PR. We can do it here and I will rebase the other branches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address the comments and get back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address the comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The one thing I am not completely satisfied about is the naming convention for the embedding options. I will try to come up with more meaningful names and we can do another update later.
@kaarthiksundar
File addition:
Data structure changes:
Algorithm changes and Function additions:
Error schemes and default changes:
Test changes:
---------- OLD Comments -------------
@harshangrjn Please see this PR for some consideration on what should be left out.
This is the all the development for
POD.jl
on the embedding formulation side #64 .It contains three main aspects,
The options introduced in this PR here are:
embedding::Bool -> Control the to use embedding formulation or not
embedding_encode::Any -> Direct which encoding will be used (compatibility will be checked)
embedding_ibs::Bool -> Whether to exploit the compactness of the formulation or not
embedding_link::Bool -> Add bound restriction constraints to regulate variable bounds based on active partition
Test cases are added to verify the encoding mapping and algorithm. Note that embedding formulation, regardless what additional options to choose, should yield the same results as SOS-2 formulation.
!!! This PR branch off #72.