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
Merged

fix promote_op function #29739

merged 1 commit into from
Oct 25, 2018

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash 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}
Copy link
Member

Choose a reason for hiding this comment

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

💯

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

Choose a reason for hiding this comment

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

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

@JeffBezanson
Copy link
Member

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

Out of curiosity, where is this code?

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
Copy link
Member Author

vtjnash 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 merged commit 10e81ce into master Oct 25, 2018
@vtjnash vtjnash deleted the jn/fix-promote-op branch October 25, 2018 04:50
@KristofferC KristofferC added the needs nanosoldier run This PR should have benchmarks run on it label Oct 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs nanosoldier run This PR should have benchmarks run on it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

promote_op doesn't play well with Union{Missing, T}
3 participants