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

Test dimensional reduce with non-bitstype #27457

Merged
merged 1 commit into from
Oct 8, 2019

Conversation

non-Jedi
Copy link
Contributor

@non-Jedi non-Jedi commented Jun 6, 2018

Fixes #26709 (see discussion on reasoning in issue)

@nalimilan
Copy link
Member

nalimilan commented Jun 25, 2018

Bump. I was going to make the same PR. @JeffBezanson Any objections?

Though I'm not sure the test needs to be so complex. A much shorter reproducer is:

julia> mapreduce(cos, +, Union{Int,Missing}[1], dims=1)
ERROR: InexactError: Int64(Int64, 0.5403023058681398)

EDIT: actually, more changes are needed to fix the above test case. I guess the only really correct solution will be to use the type of z and widen progressively if needed, as we do for map. Cf. #10533 (comment).

EDIT2: maybe as a temporary fix we could do typeof(z) <: T ? T : typeof(z)? That's really what matters here.

@nalimilan nalimilan added the status:triage This should be discussed on a triage call label Jul 7, 2018
@JeffBezanson
Copy link
Sponsor Member

maybe as a temporary fix we could do typeof(z) <: T ? T : typeof(z)? That's really what matters here.

That does sound a bit better to me --- using T can't possibly work unless z isa T.

@nalimilan
Copy link
Member

I've implemented that in the first commit of #28027.

@JeffBezanson
Copy link
Sponsor Member

Fixed by #28089, but would be good to add the test case from here.

@JeffBezanson JeffBezanson removed the status:triage This should be discussed on a triage call label Jul 13, 2018
@non-Jedi non-Jedi force-pushed the reducedim_custom_eltype branch 2 times, most recently from db27d6c to 665068e Compare October 8, 2019 15:59
@mbauman mbauman added the test This change adds or pertains to unit tests label Oct 8, 2019
@non-Jedi
Copy link
Contributor Author

non-Jedi commented Oct 8, 2019

Force-pushed this to a commit with just the test-case if there's still interest in including it.

@mbauman mbauman changed the title Allow dimensional reduce with non-bitstype Test dimensional reduce with non-bitstype Oct 8, 2019
@mbauman mbauman merged commit 80dca30 into JuliaLang:master Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test This change adds or pertains to unit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reducedim allocates array of incorrect eltype
4 participants