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

fix promote_op function #29739

Merged
merged 1 commit into from Oct 25, 2018

Conversation

3 participants
@vtjnash
Copy link
Member

commented Oct 20, 2018

This function had some weird and broken extraneous code,
using Core.Inference directly is not recommended,
but where we are going to do it anyways, we should at least do it correctly.

The cumsum code had a test to assert that Real + Real was convertable to Real.
This was hacked into the code poorly. It is now hacked in directly.
We can separately debate if this is a good idea,
I am simply preserving the existing behavior of the code
but implementing it correctly (by changing the behavior of the actual function,
instead of by convincing inference to return the incorrect answer).

close #28370 -- not really fixable, but just don't expect to get good results from inference and you should be all set.


## promotion to complex ##

_default_type(T::Type{Complex}) = Complex{Int}

This comment has been minimized.

Copy link
@JeffBezanson
if isdefined(Core, :Compiler)
const _return_type = Core.Compiler.return_type
const _unsafe_return_type = Core.Compiler.return_type

This comment has been minimized.

Copy link
@JeffBezanson

JeffBezanson Oct 20, 2018

Member

Technically we're allowed to rename this, but I believe several packages use it so we should continue to support the old name.

@JeffBezanson

This comment has been minimized.

Copy link
Member

commented Oct 20, 2018

The cumsum code had a test to assert that Real + Real was convertable to Real.

Out of curiosity, where is this code?

fix promote_op function
This function had some weird and broken extraneous code,
using Core.Inference directly is not recommended,
but where we are going to do it anyways, we should at least do it correctly.

The `cumsum` code had a test to assert that `Real + Real` was convertable to `Real`.
This was hacked into the code poorly. It is now hacked in directly.
We can separately debate if this is a good idea,
I am simply preserving the existing behavior of the code
but implementing it correctly (by changing the behavior of the actual function,
instead of by convincing inference to return the incorrect answer).
@vtjnash

This comment has been minimized.

Copy link
Member Author

commented Oct 23, 2018

Out of curiosity, where is this code?

I'm referring specifically to the addition of this method:
add_sum(x::Real, y::Real)::Real = x + y

Previously, this result was achieved by defining that the result eltype was the typejoin of the input types with the result of calling + on Int (typejoin(Real, Real, _return_type(+, (Int, Int))).

    R = promote_op(add_sum, T, T)
    out = similar(A, R)
    cumsum!(out, A, dims=dims)
    return out

Since there's a cumsum test for this, presumably it's intentional. (But that could also be a future PR to change.) I guess the assertion here is intended to imply that + must form a ring? But in that case, it seems that the promote_op call would be unnecessary altogether.

@vtjnash vtjnash force-pushed the jn/fix-promote-op branch from 67621cc to 40a4773 Oct 23, 2018

@vtjnash vtjnash merged commit 10e81ce into master Oct 25, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
julia freebsd ci Build done
Details

@vtjnash vtjnash deleted the jn/fix-promote-op branch Oct 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.