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.

Sign up for GitHub

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

Jump to bottom

WIP Add comments to solvers.jl #542

Merged
merged 10 commits into from
Aug 22, 2015
Merged

WIP Add comments to solvers.jl #542

merged 10 commits into from
Aug 22, 2015

Conversation

IainNZ
Copy link
Collaborator

@IainNZ IainNZ commented Aug 20, 2015

I hadn't looked at this stuff in a while, so while I review it I thought I'd add comments for the sake of future generations.

IainNZ
IainNZ reviewed Aug 20, 2015
View reviewed changes
src/solvers.jl
m.colVal = colVal
end
end

# The MathProgBase interface defines a conic problem to always be
# a minimization problem, so we need to flip the objective before
# reporting it to the user
if traits.conic && m.objSense == :Max
m.objVal *= -1
scale!(m.linconstrDuals, -1)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mlubin this is a NOP right because we don't support duals for conic problems, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yep

@IainNZ IainNZ changed the title Add comments to solvers.jl WIP Add comments to solvers.jl Aug 21, 2015
mlubin
mlubin reviewed Aug 21, 2015
View reviewed changes
src/solvers.jl
sos::Bool
conic::Bool
int::Bool # has integer variables
lin::Bool # has only linear constraints
Copy link
Member

Choose a reason for hiding this comment

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

and linear objective?

@IainNZ
Copy link
Collaborator Author

IainNZ commented Aug 21, 2015

Perf changes for the verify_ownership change.
Times both on 0.4, with JuMP compiled. On this branch, so using verify_ownership throughout.

BEFORE
PMEDIAN BUILD MIN=0.539766973  MED=0.829337445
PMEDIAN INTRN MIN=4.777901945  MED=4.856417003
CONT5 BUILD   MIN=0.18575696  MED=0.367095097
CONT5 INTRN   MIN=0.511391816  MED=0.535300503

AFTER
PMEDIAN BUILD MIN=0.574436618  MED=0.835707421
PMEDIAN INTRN MIN=4.345520664  MED=4.43239901
CONT5 BUILD   MIN=0.183608798  MED=0.310254336
CONT5 INTRN   MIN=0.472166626  MED=0.519696314

So pretty marginal, but something (maybe)

IainNZ added a commit that referenced this pull request Aug 22, 2015
@IainNZ
Merge pull request #542 from JuliaOpt/comments
f5d381c 
WIP Add comments to solvers.jl
@IainNZ IainNZ merged commit f5d381c into master Aug 22, 2015
@IainNZ IainNZ deleted the comments branch August 22, 2015 05:26
@IainNZ
Copy link
Collaborator Author

IainNZ commented Aug 22, 2015

I'll probably keep going with this, because its turning up some real stuff, but this PR was going to get a bit long (even with clean discrete commits)

mlubin
mlubin reviewed Aug 22, 2015
View reviewed changes
src/JuMP.jl
function verify_ownership(m::Model, vec::Vector{Variable})
n = length(vec)
@inbounds for i in 1:n
!isequal(vec[i].m, m) && return false
Copy link
Member

Choose a reason for hiding this comment

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

why !isequal instead of !===?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Shrug, @joehuchette ? I just copied what was there

@YDHsieh YDHsieh mentioned this pull request Apr 18, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@IainNZ @mlubin