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

Norms! #465

Merged
merged 1 commit into from Jul 28, 2015
Merged

Norms! #465

merged 1 commit into from Jul 28, 2015

Conversation

joehuchette
Copy link
Contributor

Still need to plug the solvers in.

# Container for expressions containing Norms and AffExprs
type NormExpr
norm::Norm
coeff::Float64
Copy link
Member

Choose a reason for hiding this comment

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

What is coeff? Needs more comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

coeff*||norm.terms|| + aff

Copy link
Member

Choose a reason for hiding this comment

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

Okay, just add that as a comment :)

@mlubin
Copy link
Member

mlubin commented Jun 14, 2015

Nice! What do you think about switching to the conic interface if norms are present? I guess this would need the SDP branch merged in first.

@joehuchette
Copy link
Contributor Author

I was thinking this gets merged before SDP, so I can add the conic stuff for SOC there. The default solver stuff for conic problems needs to be rethought at some point.

@mlubin
Copy link
Member

mlubin commented Jun 14, 2015

Sounds good.

@mlubin
Copy link
Member

mlubin commented Jun 14, 2015

Also, do we need to store the coeff term? Why not just distribute it into the norm?

@joehuchette
Copy link
Contributor Author

If it's negative, we want to recognize that something like -||x^2+y^2|| >= -1 is SOC representable.

@mlubin
Copy link
Member

mlubin commented Jun 14, 2015

Or we could not and let users write it with a positive coefficient. We're not Convex.jl after all

@joehuchette
Copy link
Contributor Author

With the way we parse expressions, I think something like 1 <= norm([x,y]) might get rewritten that way regardless

@mlubin
Copy link
Member

mlubin commented Jun 14, 2015

Fair enough

@IainNZ
Copy link
Collaborator

IainNZ commented Jun 14, 2015

Any hope for other norms? Or are we committing ourselves to L2?

@joehuchette
Copy link
Contributor Author

It should be pretty easy to make Norm parametric and handle arbitrary norms.

@tkelman
Copy link
Contributor

tkelman commented Jun 14, 2015

1 and inf norms would be super useful (yes I know it's easy to reformulate those but the code looks prettier when I don't have to)

@joehuchette
Copy link
Contributor Author

Agreed. What would nice syntax look like? norminf and norm1?

@tkelman
Copy link
Contributor

tkelman commented Jun 14, 2015

That would probably be good enough, yeah. Extra args inside norm{foo, p} might be ambiguous with other ways you use commas inside braces. 1, 2, and inf norms should cover most use cases to start with.

@mlubin
Copy link
Member

mlubin commented Jun 14, 2015

I would make the syntax something like norm2{} instead of norm{}.

@IainNZ
Copy link
Collaborator

IainNZ commented Jun 14, 2015

This is some back-seat designing, but could be good to have NormConstraint{T} be a constraint on a Norm{T}, and then its m.normconstrs etc.
This is going to be a nice addition.

@joehuchette
Copy link
Contributor Author

Good point, that doesn't generalize nicely with arbitrary norms.

@@ -474,6 +478,40 @@ end
getValue(arr::Array{QuadExpr}) = map(getValue, arr)

##########################################################################
# Norm
# Container for √(∑ expr)
type Norm{T}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, can we make this parametric on the expression:

type Norm{T,E<:GenericAffExpr}
    terms::Vector{E}
end

So I can use it for JuMPeR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing

@iamed2
Copy link
Contributor

iamed2 commented Jun 25, 2015

+1

@joehuchette joehuchette force-pushed the norms! branch 2 times, most recently from 73f3f68 to 0623f06 Compare June 30, 2015 17:25
@tkelman
Copy link
Contributor

tkelman commented Jul 12, 2015

cc @smukoehler

@mlubin
Copy link
Member

mlubin commented Jul 14, 2015

@joehuchette, could you rebase?

for (mac,sym) in [(:LinearConstraints, symbol("@LinearConstraint")),
(:QuadConstraints, symbol("@QuadConstraint"))]
(:QuadConstraints, symbol("@QuadConstraint")),
(:SOCConstraints, symbol("@SOCConstraint"))]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Alignment, if you care

@mlubin
Copy link
Member

mlubin commented Jul 27, 2015

I think this needs an update for the copy changes?
I just reran travis and got a failure: https://travis-ci.org/JuliaOpt/JuMP.jl/jobs/71783825#L429

@mlubin
Copy link
Member

mlubin commented Jul 27, 2015

Nvm, master is broken

@joehuchette
Copy link
Contributor Author

OK, I'm gonna merge this in the next day or so if there aren't any outstanding concerns. Operator coverage should be 100%, and the printing code is ugly but functional.

@joehuchette
Copy link
Contributor Author

Conic duals don't work either, but I think that'll take more work throughout the stack that's sufficiently orthogonal from this PR.

@defVar(m, L1[1:d])
@addConstraint(m, L1 .== (μ-μhat))
@addConstraint(m, sum{L1[i]^2, i=1:d} <= t1^2)
@addConstraint(m, t1 <= Γ1(𝛿/2,N))
Copy link
Member

Choose a reason for hiding this comment

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

Could we leave these in as a separate test? Always good to test multiple ways of doing the same thing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, sure

@mlubin
Copy link
Member

mlubin commented Jul 27, 2015

LGTM.
I do think we should have duals for the 0.10 release, at least for the conic solvers which support it.

@mlubin
Copy link
Member

mlubin commented Jul 28, 2015

Printing tests are failing on travis

@joehuchette
Copy link
Contributor Author

Yeah, it's gonna be a mess to fix...

@mlubin
Copy link
Member

mlubin commented Jul 28, 2015

What's wrong?

@joehuchette
Copy link
Contributor Author

It looks like p[end] and last(p) don't work for p::Pair. We'll probably have to rewrite JuMPContainer iteration to match, as well.

@mlubin
Copy link
Member

mlubin commented Jul 28, 2015

Is this a recent change in master or just this branch?

@joehuchette
Copy link
Contributor Author

Master as well, I'm pretty sure

@mlubin
Copy link
Member

mlubin commented Jul 28, 2015

Oh fun. Maybe we can propose to add these definitions for Pair?

@mlubin
Copy link
Member

mlubin commented Jul 28, 2015

Pair doesn't really fit with our iteration syntax either.

@joehuchette
Copy link
Contributor Author

JuliaLang/julia#12335

@joehuchette
Copy link
Contributor Author

I can add endof too

@joehuchette
Copy link
Contributor Author

OS X release is failing Travis because of an old build, I ran it locally and everything passes.

joehuchette added a commit that referenced this pull request Jul 28, 2015
@joehuchette joehuchette merged commit b2adb58 into master Jul 28, 2015
@mlubin
Copy link
Member

mlubin commented Jul 28, 2015

💯

@IainNZ
Copy link
Collaborator

IainNZ commented Jul 28, 2015

Good stuff Joey!

@@ -105,6 +105,17 @@ you would a linear objective::

status = solve(m)

Second-order cone constraints
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be in the "Expressions and Constraints" section?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that'd make sense

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants