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

DNMY: rely on promote_op for many operations #604

Closed
wants to merge 1 commit into from
Closed

DNMY: rely on promote_op for many operations #604

wants to merge 1 commit into from

Conversation

timholy
Copy link
Contributor

@timholy timholy commented Oct 28, 2015

Requires backporting of JuliaLang/julia#13803, do not merge yet. Once it gets backported, do not merge until the REQUIRE file has its julia version bumped. (I'm not sure which julia version it will end up in, or I'd add that change now.)

I noticed a few "Array"-type operations missing. While it would have been easier to write specialized methods to handle the operations I needed, it seemed best to try to get to the point where we supply the right traits and then rely on methods in Base.

@mlubin
Copy link
Member

mlubin commented Oct 28, 2015

Very cool, thanks! Ref #179
Could we also shift to this approach for scalar operations?

@joehuchette
Copy link
Contributor

This looks great, thanks!

@@ -465,7 +465,7 @@ module TestHelper # weird scoping behavior with FactCheck...
return x
end

vec_eq(x,y) = vec_eq([x;], [y;])
vec_eq(x,y) = vec_eq(convert(Array, [x;]), convert(Array, [y;]))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this change needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isa(x, SparseMatrixCSC) == true. If you leave it as a sparse matrix, you get a stack overflow because all other methods are implemented only for Array and so this method gets called again.

@timholy
Copy link
Contributor Author

timholy commented Oct 28, 2015

I think this basically closes #179.

I'm not exactly certain what you mean by shifting for scalar operations; unfortunately right now you have to basically implement scalar operations for both instances and types, e.g.,

(+)(lhs::Number, rhs::GenericAffExpr) = GenericAffExpr(copy(rhs.vars),copy(rhs.coeffs),lhs+rhs.constant)
Base.promote_op{R<:Real}(::AddFun, ::Type{R}, ::Type{AffExpr}) = AffExpr

But all those are scalar objects. Once you do that, the array operations follow automatically. (And you really only need the type-implementation for use in the array operations.)

Someday, someone will write a ReturnTypes.jl package that is an independent implementation of type-inference. Once it's working well, many of us will lobby to put this in base, and if that happens then we won't need the type-implementation. See the extensive discussion in JuliaLang/julia#12292 (comment).

@mlubin
Copy link
Member

mlubin commented Oct 28, 2015

@timholy, I mean falling back on promotion rules to convert Number + AffExpr into AffExpr + AffExpr. Though there's some loss of efficiency there.

@timholy
Copy link
Contributor Author

timholy commented Oct 28, 2015

Oh, now I understand. That could be good for + and -, but (to state what's certainly obvious to you) I think for * you'd lose important type information, since Number*AffExpr -> AffExpr and not QuadExpr. Not sure it would be worth it for just + and -.

@mlubin
Copy link
Member

mlubin commented Dec 11, 2016

@timholy, any interest in updating this PR?

@mlubin
Copy link
Member

mlubin commented Jun 9, 2017

This is stale, but we still have #179 open

@mlubin mlubin closed this Jun 9, 2017
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.

3 participants