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

Ensure that collect(::AbstractArray) always returns an Array #21257

Merged
merged 2 commits into from
Apr 5, 2017

Conversation

timholy
Copy link
Member

@timholy timholy commented Apr 2, 2017

The docstring specifies that the return type is Array, but similar isn't guaranteed to return an Array. (Exceptions include AxisArrays, OffsetArrays, etc.)

This is both a bugfix and a behavior change. I'll leave it to others to decide whether this can be merged in 0.6.

The docstring specifies that the return type is `Array`, but `similar` isn't guaranteed to return an `Array`. (Exceptions include AxisArrays, OffsetArrays, etc.)
@ararslan ararslan added the arrays [a, r, r, a, y, s] label Apr 2, 2017
@timholy
Copy link
Member Author

timholy commented Apr 3, 2017

@tkelman, to determine whether the behavior change would break anything, perhaps we could have a PkgEval run of this? (With regards to merging in 0.6, the "behavior change" situation here is similar to #20720. Though in this case there's a docstring saying how it is supposed to behave, but isn't.)

@tkelman
Copy link
Contributor

tkelman commented Apr 3, 2017

Sure, fired that off now, should be done later today. Once I have a good place hooked up to run these I should get it set up so we can trigger comparison runs automatically.

@tkelman
Copy link
Contributor

tkelman commented Apr 3, 2017

https://gist.github.com/21f2c1ad2ea384895b6cffcef1d5d530
ImageCore might just be a too-tight tolerance? And I think ReverseDiff timed out and was terminated on the baseline run.

@timholy
Copy link
Member Author

timholy commented Apr 4, 2017

That's awesome. The ImageCore failure was just in benchmarks, not functionality (and the failure is almost certainly noise, this PR should have no impact on that test).

So it seems like this is unlikely to hurt anyone. Unless objections arise, I'd propose we merge this for 0.6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants