Implement value for GenericQuadExpr. #1588
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1588 +/- ##
==========================================
+ Coverage 90.8% 90.95% +0.15%
==========================================
Files 28 28
Lines 3696 3769 +73
==========================================
+ Hits 3356 3428 +72
- Misses 340 341 +1
Continue to review full report at Codecov.
|
src/quad_expr.jl
Outdated
| # Later, we're going to multiply two MapVarType together | ||
| MapVarType2 = Base.promote_op(*, MapVarType, MapVarType) | ||
| # We're also going to multiply a constant with ::MapVarType2 | ||
| RetType = Base.promote_op(*, CoefType, MapVarType2) |
There was a problem hiding this comment.
I was once told that it is not safe to call promote_op on top of promote_op, see JuliaLang/julia#18218 (comment).
So maybe something like RetType = Base.promote_op((c, v) -> c * foo(v) * foo(v), CoefType, VarType) is better.
There was a problem hiding this comment.
There was a problem hiding this comment.
I never understood the argument so I am not surprised to see that I am not following it :-P
In fact, it appears that using promote_op is just bad: JuliaLang/julia#26344 (comment)
So the best way is just to play with typeof, zero, one and oneunit. To choose whether to use one or oneunit you need to think what would happen if the type was frim Unitful. For choosing between zero and one, you have some freedom but you need to avoid dividing by zero.
Maybe the best way here is doing typeof(oneunit(CoefType) * oneunit(MapVarType) * oneunit(MapVarType)). We are kind of forced to use promote_op to get MapVarType since one ... would on JuMP.VariableRef would not really do what we want.
There was a problem hiding this comment.
I think we have to use promote_op here because the variables get passed through foo...
Alternatively, we just avoid the value result_value misdirection and write a result_value function knowing that it will get a Float64 back...
Closes #1563