[release-0.18] light at the end of the tunnel #1432

ExpandingMan · 2018-08-21T19:10:16Z

Hello all. I apologize for the gargantuan size of this PR, but I was on a role and it got away from me.

I've made a large number of fixes to get this running on 1.0. All non-solver-dependent tests now pass on 1.0 with two exceptions

There as a block (in print.jl) that I took out completely, because it was extremely pedantic and had been taken out in master (even though the functionality still exists in master).
There seem to be various errors with OffsetArrays. According to the stack traces these seem to be entirely contained to OffsetArrays. For now I have commented out a few (I think 3) tests that use OffsetArrays.

Most of the fixes here involve macros or operators. I have taken as much code from @mlubin 's fixes to master as possible. Fortunately this was quite a lot.

Obviously the operator code is now a mess and needs major refectoring (for arrays, scalars should be fine). I'm pretty sure what's here now should be about as good as what's in master. I now have lots of good ideas for an operator PR to master, so I hope to get to that relatively soon.

ExpandingMan · 2018-08-22T01:31:22Z

Ok, I have fixed an absolutely bizarre @generated function related bug on 0.6, think this is it.

All that remains is to get the solver-dependent tests running, but with any luck that should mostly just be a matter of getting those packages updated and tagged.

Once we go through review and make changes, if you approve let's merge and finish solvers on a separate PR.

blegat · 2018-08-22T07:36:00Z

src/affexpr.jl

+# variables must be ∈ ℝ but coeffs can be ∈ ℂ
+Compat.adjoint(a::GenericAffExpr{<:Real}) = a
+function Compat.adjoint(a::GenericAffExpr)
+    GenericAffExpr(a.vars, [z' for z ∈ a.coeffs], a.constant')


Could you use adjoint. (a.coeffs) and adjoint (constant) ? It is more readable IMO as it does not require to remember that ' is not transpose but adjoint

That would create an Adjoint{Vector} rather than a Vector, so no, you can't do that.

Actually, yes you can do it with the dot operator, didn't notice your dot. Ok, I'll do that.

blegat · 2018-08-22T07:38:30Z

src/quadexpr.jl

+# variables are ∈ ℝ but coeffs can be ∈ ℂ
+Compat.adjoint(q::GenericQuadExpr{<:Real}) = q
+function Compat.adjoint(q::GenericQuadExpr)
+    GenericQuadExpr(q.qvars1, q.qvars2, [z' for z ∈ q.qcoeffs], aff')


Same comment here

blegat · 2018-08-22T15:31:54Z

src/operators.jl

+# this is a generic fallback from stdlib LinearAlgebra that's needed for tests to pass on 0.6
+# for example, dot(::Array{Float64,3}, ::Array{Int,3}) is not defined in 0.6
+if VERSION < v"0.7-"
+    function Base.dot(x::AbstractArray, y::AbstractArray)


This is type piracy if we define a method for a Base function, it should be with custom types. Can't we implement this in a custom function like _dot ?

Sorry, I think I just started getting too desparate to get 0.6 working.

The proper solution would probably just be to ensure in the tests that this the arrays are promoted properly first. I'm not entirely sure why this wasn't causing a problem in the original 0.18. Let me look into it a bit. I don't think it would be unreasonable to change the test to ensure that promotion occurs, what do you think?

Looking this I am more and more confident that the original 0.18 must have been commiting type piracy. Regardless, I think the fixes to the tests will be pretty simple so we'll all be much happier with that.

Which line of the tests is making it fail ?

codecov · 2018-08-22T16:01:40Z

Codecov Report

Merging #1432 into release-0.18 will decrease coverage by 1.87%.
The diff coverage is 62.89%.

@@               Coverage Diff                @@
##           release-0.18    #1432      +/-   ##
================================================
- Coverage         90.41%   88.54%   -1.88%     
================================================
  Files                18       18              
  Lines              4623     4715      +92     
================================================
- Hits               4180     4175       -5     
- Misses              443      540      +97

Impacted Files	Coverage Δ
src/parseExpr_staged.jl	`93.97% <100%> (+0.27%)`	⬆️
src/JuMPContainer.jl	`70.51% <100%> (ø)`	⬆️
src/solvers.jl	`94.48% <100%> (ø)`	⬆️
src/utils.jl	`90.38% <100%> (-0.19%)`	⬇️
src/quadexpr.jl	`69.35% <33.33%> (-1.84%)`	⬇️
src/operators.jl	`78.3% <40.56%> (-16.57%)`	⬇️
src/parsenlp.jl	`68.07% <61.53%> (-0.68%)`	⬇️
src/print.jl	`91.83% <66.66%> (-1.08%)`	⬇️
src/affexpr.jl	`84.09% <66.66%> (-0.62%)`	⬇️
src/JuMP.jl	`89.82% <69.23%> (-0.5%)`	⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b412370...ee27487. Read the comment docs.

ExpandingMan · 2018-08-22T16:02:51Z

Ok, sorry that was really silly of me, should be better now. (Deep down I just think type piracy is cool ☠️ 😆 )

For the record, check out test/operator.jl line 510. Now figure that one out: how could that have been working before without type piracy? Regardless, it should be fine now.

blegat · 2018-08-22T16:33:48Z

src/operators.jl

-# TODO: implement sparse * sparse code as in base/sparse/linalg.jl (spmatmul)
-_multiply!(ret::AbstractArray{T}, lhs::SparseMatrixCSC, rhs::SparseMatrixCSC) where {T<:JuMPTypes} = _multiply!(ret, lhs, full(rhs))
-_multiplyt!(ret::AbstractArray{T}, lhs::SparseMatrixCSC, rhs::SparseMatrixCSC) where {T<:JuMPTypes} = _multiplyt!(ret, lhs, full(rhs))
+function Base.Matrix(S::SparseMatrixCSC{V}) where V<:Variable


It is needed because of this bug right: https://github.com/JuliaLang/julia/issues/27015 ?

I believe that's one of the reasons. Also remember there's no dense in 1.0, so you need a safe way of converting to a dense matrix.

It was added in master to fix #1275. and there is a comment See https://github.com/JuliaLang/julia/issues/27015 explaining why it is needed on master above the method. I think we should add it here too

ExpandingMan · 2018-08-22T16:42:16Z

By the way, as you review please bear in mind src/operators.jl in the current master. I realize I changed the 0.18 operators extensively, but I tried hard to stick as close to 0.19 as possible (and stole the code in most cases).

blegat · 2018-08-22T17:57:01Z

test/operator.jl

@@ -628,9 +634,9 @@ Base.transpose(t::MySumType) = MySumType(t.a)
        @test vec_eq(@JuMP.Expression(A*x/2), A*x/2)
        @test vec_eq(X*v,  [4X11; 6X23; 0])
        @test vec_eq(v'*X,  [4X11  0   5X23])
-        @test vec_eq(v.'*X, [4X11  0   5X23])
+        @test vec_eq(transpose(v)*X, [4X11  0   5X23])


Isn't .' equivalent to transpose ?

It seems it has been removed, never mind :)

mlubin

I'm impressed that you got this working.
I took a look at the CI logs for 0.7 and they print the addition operator warning thousands of times to the extent that travis won't display the rest of the output (https://travis-ci.org/JuliaOpt/JuMP.jl/jobs/419286826#L2087). This isn't maintainable. Is there an easy way to fix?

mlubin · 2018-08-23T02:40:18Z

src/nlp.jl

@@ -530,7 +530,7 @@ function MathProgBase.eval_grad_f(d::NLPEvaluator, g, x)
 end

 function MathProgBase.eval_g(d::NLPEvaluator, g, x)
-    tic()
+    VERSION < v"0.7-" && tic()


Why are the tic()s version protected but not the toq()s? Does this code work?

I think the toqs are in the other packages no?

The easiest thing to do will be to disable them in those packages also in 0.7, the only other option is to re-implement the feature ourselves.

What other packages? I'm confused because both tic and toq are deprecated, and it's an error to run toq without a corresponding tic.

Hm, I think for some reason I though they were in the solvers or MathProgBase. I'm not sure how it is that they are not being called in tests but tic is. I'll look into it.

This is a bit tricky. I think the only good option here is to re-implement tic toq in 1.0. I don't think we can take this out completely as it looks like some of the MathProgBase logic may depend on it. Errors in tests were probably burried in the 1.0 solver errors.

ExpandingMan · 2018-08-23T02:50:35Z

Hm, something is wrong. It's not supposed to print that warning more than once. The new logging functionality has a max warnings feature, and I'm using it. A bug in the logger maybe? I'll look into it.

For what it's worth, getting this working would not even have been particularly difficult if it were not for the fact that we had to continue to support 0.6. It's extremely unfortunate that we were in the position of having to support it when no features will be added for it. I know the situation JuMP was in here was just a lot of bad luck in timing, but I hope they drop the METADATA restriction where you can't change language versions in minor package version numbers. It really caused a lot of grief here (whether or not I was masochistic to take this on in the first place).

IainNZ · 2018-08-23T03:04:13Z

JuliaLang/julia#28786 This maybe?

ExpandingMan · 2018-08-23T03:05:34Z

Yup, probably.

mlubin · 2018-08-23T03:15:00Z

Huh? There's no logic in MPB that depends on tic/toc. They're used only as timers within the scope of a single function.

…

On Wed, Aug 22, 2018, 23:09 ExpandingMan ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/nlp.jl <#1432 (comment)>: > @@ -530,7 +530,7 @@ function MathProgBase.eval_grad_f(d::NLPEvaluator, g, x) end function MathProgBase.eval_g(d::NLPEvaluator, g, x) - tic() + VERSION < v"0.7-" && tic() This is a bit tricky. I think the only good option here is to re-implement tic toq in 1.0. I don't think we can take this out completely as it looks like some of the MathProgBase logic may depend on it. Errors in tests were probably burried in the 1.0 solver errors. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1432 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABp0M29hYYm07jtwc4JaMytOT0zs_1qtks5uThzsgaJpZM4WGbLB> .

ExpandingMan · 2018-08-23T03:19:19Z

I was referring to MathProgBase.initialize in nlp.jl line 240. It checks whether the timer is zero to see whether it's already been initialized and exit.

Regardless, sorry, I didn't get a chance to look at it carefully yet. When I get a chance I'll look at it carefully and come up with a solution.

Any objections to abolishing the timer in favor of a simple flag, or will something else rely on it somewhere?

mlubin · 2018-08-23T03:25:05Z

Yes, it's fine to remove the timers.

…

On Wed, Aug 22, 2018, 23:19 ExpandingMan ***@***.***> wrote: I was referring to MathProgBase.initialize in nlp.jl line 240. It checks whether the timer is zero to see whether it's already been initialized and exit. Regardless, sorry, I didn't get a chance to look at it carefully yet. When I get a chance I'll look at it carefully and come up with a solution. Any objections to abolishing the timer in favor of a simple flag, or will something else rely on it somewhere? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1432 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABp0MwEQgmp151P3cEbuOJ7_dNF-Su-Eks5uTh84gaJpZM4WGbLB> .

mlubin · 2018-08-24T00:19:06Z

src/nlp.jl

-    eval_hesslag_timer::Float64
+    # init flags
+    eval_f_init::Bool
+    eval_g_init::Bool


Are any of these flags used except for eval_f_init? Please remove them if not.

Apparently they all get set, but whether they actually get used for anything I don't know (probably not within JuMP). Perhaps best to just leave it as this is only 0.18.

And when I say "they all get set" I mean there are other functions that set them, not just when they are initialized in the constructors.

ExpandingMan · 2018-08-24T01:01:49Z

Ok, I have:

Replaced the timers with simple flags (on both 0.6 and 1.0).
Temporarily commented out the addition warning. There don't seem to be any good solutions to this while the Julia logger seems to have a bug. If you want I can put a global const WARN_FLAGS = Dict(:warning_name=>f::Bool) to emulate the warn once behavior as a temporary solution.

mlubin · 2018-08-26T19:05:50Z

Please re-enable the warning on 0.6 so we're not introducing a regression. The warning is pretty useful, and there will be people using the 0.18 branch on 0.6 for quite a while.

After that looks good to merge from my end.

ExpandingMan · 2018-08-27T02:20:14Z

With any luck most of the remaining work is just getting the various solvers stable on 1.0. Any tagging you feel you can do would be greatly appreciated, as I know the masters of a few of them are working on 1.0 (or at least 0.7) but not tagged.

mlubin · 2018-08-27T02:28:39Z

We'll do a round of solver tagging soon (after MOI 0.6 is released). Right now it would just create more of a mess of incompatible versions.

ExpandingMan added 8 commits August 16, 2018 16:47

fixed some more AST issues in the macros

7bee8ef

some more macro fixes

6265a58

a lot more fixes to macros

5138462

getting closer

402369a

lots of cleaning up operators

503b104

1.0 now passes

217e8cb

fixed annoying failure on 0.7 due to AST change

558cf85

fixed bizarre generated function bug on 0.6

b63ea7a

restored tics on 0.6

811edc3

blegat reviewed Aug 22, 2018

View reviewed changes

ExpandingMan added 3 commits August 22, 2018 10:15

fixed wrong dot extension

edd8534

added dot fallback for 0.6

84640a7

attempt to fix method error in custom type test

348f882

blegat reviewed Aug 22, 2018

View reviewed changes

removed silly type piracy

40ba800

blegat reviewed Aug 22, 2018

View reviewed changes

added comment referencing issue

747adc6

blegat reviewed Aug 22, 2018

View reviewed changes

blegat approved these changes Aug 22, 2018

View reviewed changes

mlubin reviewed Aug 23, 2018

View reviewed changes

IainNZ mentioned this pull request Aug 23, 2018

in warn macro the maxlog keyword is not effective JuliaLang/julia#28786

Closed

replaced timers

66425fa

mlubin reviewed Aug 24, 2018

View reviewed changes

temporarily disabled addition warning

56bea32

restored addition warning in 0.6

ee27487

mlubin merged commit 101ba7e into jump-dev:release-0.18 Aug 27, 2018

mlubin mentioned this pull request Sep 28, 2018

[release-0.18] Readd GLPK #1498

Merged

blegat mentioned this pull request Sep 28, 2018

[release-0.18] Segfault on Julia v0.6 #1501

Closed

[release-0.18] light at the end of the tunnel #1432

[release-0.18] light at the end of the tunnel #1432

Conversation

ExpandingMan commented Aug 21, 2018 • edited Loading

ExpandingMan commented Aug 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 22, 2018 • edited Loading

Codecov Report

ExpandingMan commented Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ExpandingMan commented Aug 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlubin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ExpandingMan commented Aug 23, 2018 • edited Loading

IainNZ commented Aug 23, 2018

ExpandingMan commented Aug 23, 2018

mlubin commented Aug 23, 2018 via email

ExpandingMan commented Aug 23, 2018

mlubin commented Aug 23, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ExpandingMan commented Aug 24, 2018

mlubin commented Aug 26, 2018

ExpandingMan commented Aug 27, 2018 • edited Loading

mlubin commented Aug 27, 2018 • edited Loading

ExpandingMan commented Aug 21, 2018 •

edited

Loading

codecov bot commented Aug 22, 2018 •

edited

Loading

ExpandingMan commented Aug 22, 2018 •

edited

Loading

ExpandingMan commented Aug 23, 2018 •

edited

Loading

ExpandingMan commented Aug 27, 2018 •

edited

Loading

mlubin commented Aug 27, 2018 •

edited

Loading