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

Deprecate similar(f, …) in favor of just dispatching directly on f(…) #26733

Merged
merged 5 commits into from
Apr 13, 2018

Conversation

mbauman
Copy link
Member

@mbauman mbauman commented Apr 6, 2018

Fixes #26433.

Previously, OffsetArrays used similar(dims->zeros(dims), axes) to add offset support for the base functions that created arrays: zeros, ones, trues, falses, and fill. It was a bit of a strange construct — the function specified the "internal" array around which OffsetArrays would wrap. This deprecates that similar method in favor just structuring the method tables of those five functions in a manner that is conducive for overloading with offset axis argument(s). Paired with JuliaArrays/OffsetArrays.jl#43 to match the changes in test/TestHelpers.jl.

mbauman added a commit to mbauman/OffsetArrays.jl that referenced this pull request Apr 6, 2018
@mbauman mbauman added arrays [a, r, r, a, y, s] deprecation This change introduces or involves a deprecation labels Apr 6, 2018
@mbauman
Copy link
Member Author

mbauman commented Apr 6, 2018

Just a note that I first attempted to coalesce all these functions around an AbstractArray{T}(dims_or_axes…) constructor. I still think that may be a good idea, but it was turning into a rabbit hole and is a new feature so I decided to punt on it until later. I left that first commit here that does so — it can be squashed out on merge but I think it's nice to leave here in the GitHub PR as a starting point if we ever decide to implement that feature.

The biggest reason not to couple it with this change was simply that it became quite disruptive — it meant that fill(true, dims) returned a BitArray. That led me down quite a rabbit hole trying to figure out how we could come up with a consistent rule for returning arrays or bitarrays… and it's not something that I particularly wanted to tackle so close to a release.

the tightening of signatures for zeros and ones.
@mbauman
Copy link
Member Author

mbauman commented Apr 6, 2018

I've added NEWS and deprecations for the newly-tighter ones and zeros methods… but now I'm running into a test failure I don't quite understand:

f5575() = zeros(Type[Float64][1], 1)
@test Base.return_types(f5575, ())[1] == Vector

With these additional deprecations, I'm now getting Any from return_types. I'm not entirely sure what that test is meant to be testing… as it appears we again have "too many methods" (#5575)? It's a little odd, as (::Type, ::Integer) is still only selecting one method:

julia> methods(zeros, Tuple{Type, Int})
# 1 method for generic function "zeros":
[1] zeros(::Type{T}, dims::Union{Integer, AbstractUnitRange}...) where T in Base at array.jl:401

So I'm not sure what to do with that...

@vtjnash
Copy link
Member

vtjnash commented Apr 10, 2018

The zeros function (currently) recurses aggressively. The test is there to make sure the compile stops trying to follow the possible branches in a reasonable time, without losing accuracy.

@mbauman
Copy link
Member Author

mbauman commented Apr 10, 2018

Thanks. So the fact that I broke this test by adding these deprecations is expected? Should I just copy the previous zeros method tree (without deprecations) onto a new function that gets defined in the test and whose sole purpose is this one test?

that has the known properties that we want to test. The important thing is not necessarily zeros -- it is a relatively complex function. The `Base.zeros` method tree temporarily breaks this test due to its numerous deprecations, but this will be resolved in the future.
@mbauman mbauman added the triage This should be discussed on a triage call label Apr 11, 2018
@StefanKarpinski StefanKarpinski removed the triage This should be discussed on a triage call label Apr 12, 2018
@StefanKarpinski StefanKarpinski added this to the 1.0 milestone Apr 12, 2018
@JeffBezanson JeffBezanson merged commit 2233ec4 into master Apr 13, 2018
@JeffBezanson JeffBezanson deleted the mb/deprecatesimilarf branch April 13, 2018 15:28
mbauman added a commit to mbauman/OffsetArrays.jl that referenced this pull request May 9, 2018
mbauman added a commit to mbauman/OffsetArrays.jl that referenced this pull request May 10, 2018
mbauman added a commit to JuliaArrays/OffsetArrays.jl that referenced this pull request Jun 6, 2018
* Update in anticipation of deprecating similar(f, ...)

Requires JuliaLang/julia#26733

* Add VERSIONing
ron-wolf added a commit to ron-wolf/julia that referenced this pull request Aug 27, 2020
mbauman pushed a commit that referenced this pull request Aug 27, 2020
simeonschaub pushed a commit to simeonschaub/julia that referenced this pull request Aug 29, 2020
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] deprecation This change introduces or involves a deprecation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants