Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[WIP] Evaluation distance to set #1023
[WIP] Evaluation distance to set #1023
Changes from 16 commits
b606dc3
6670920
93ac7f9
55ed9d6
09663dd
9d4def8
c02e1ba
eea190e
3a78641
016fa9d
46b6a39
a1d2f6f
90d3631
ae2c2a8
b3678b5
73aaa3b
3cab392
83a6131
c969068
891426f
5cde8c8
b6f979a
b19f884
ed1d33b
f80f399
54ff584
68888b5
21c759f
9a2e898
13725c3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
A vector of distances isn't a distance, mathematically speaking. What is meant here precisely?
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.
I guess we want
distance_to_set(point, set::AbstractSet; norm::Int = 2)
. Or even just say that we're using the L2 norm.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 intuition I had for it is giving an element-wise distance when it makes sense, giving which element to "improve" to reduce the distance to the set.
We can also always normalize as @odow mentions
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.
If we add the distance type parameter we can easily allow the user to switch from the norm2 thing to a norm infinity which is pretty reasonable
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 still an issue.
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 the comment
# avoids sqrt
for?I have a general unresolved concern over our choice of distance. It seems like this is the "epigraph violation" distance, which is not the same as the Euclidean distance to the set. What is the precedent for how other solvers measure constraint feasibility? (@chriscoey / @lkapelevich how does Hypatia?)
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.
I personally use the epigraph violation
d = x1 - norm(x[2:end], 2)
, mainly becausex2^2 + ... xn^2 <= x1^2
, the epigraph violation is the square root of this quadratic constraint's absolute violation.Looks like COSMO uses euclidean distance for computing projections.
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 about sets like the exponential cone needing domain constraints like
y > 0
?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.
legacy comment, just removed. For domain constraints it can be an open question, because some parts of the distance may be non-computable / infinite or complex if the domain is not respected
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, so for conic sets "Epigraph violation or
Inf
if point fails feasibility bounds" seems like an easily computable and sensible definition.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.
And you call it EpigraphDistance or something like...
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.
For norm two distances you can use what proximal solvers do:
https://web.stanford.edu/~boyd/papers/pdf/prox_algs.pdf page 183 (section 6.3)
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.
So, SOC is easy, PSD you just need eigen decomposition, Power and Exponential are well posed even with the extra constraints, but need a Newton...
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.
I'm not an expert on this, but I know that computing the full SVD is an extremely expensive way to compute the smallest singular value. This is probably not suitable for non-toy applications. There are a few packages that do this, e.g., https://jutho.github.io/KrylovKit.jl/latest/man/svd/. We should evaluate which, if any, are suitable to add as dependencies of MOI.
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.
agreed, I just kept the dependency-free version for now. MOI can either add a dependency or vendor (copy and acknowledge) a single function from another package
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.
I don't think this function is correct. Something like: