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

Add `keytype`+`valtype` method for AbstractArray and AbstractVector #27749

Merged
merged 13 commits into from Apr 4, 2019

Conversation

@tkluck
Copy link
Contributor

commented Jun 23, 2018

The function keytype(a) returns eltype(keys(a)) for associative structures; it should do the same for AbstractArray and AbstractVector. Looks like this was overlooked(?) in #25577.

#25104 is a related pull request, but renaming is less urgent than adding expected behaviour.

Add keytype method for AbstractArray and AbstractVector
The function `keytype(a)` returns `eltype(keys(a))` for associative
structures; it should do the same for `Abstract{Array, Vector}`.
@Keno

This comment has been minimized.

Copy link
Member

commented Jun 24, 2018

eltype(keys(a))

Wouldn't literally this definition be a better generic fallback and cover these cases?

@andyferris

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2018

Agree that keytype should work but @Keno’s suggestion seems pretty fool-proof to me.

I think one stumbling block for arrays in the past is that they have at least two kinds of keys (linear and Cartesian) plus whatever we call it when you can getindex with an (arbitrarily long) varargs of integers. But I think of the definitions here as the “real” keys, and the others are “convenience” keys. But maybe that’s just me.

@tkluck

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2018

eltype(keys(a))

Wouldn't literally this definition be a better generic fallback and cover these cases?

Here's what made me not do that: keys may be implemented by doing an allocation. For example, it returns a KeySet for Dict. In this particular case, the compiler may be able to optimize it out, but that may not be true for user-defined containers.

And we'd need to implement it case-by-case for keytype on the type anyway, as we can't call keys on the type. (I now realize this pull request doesn't address that yet and it probably should.)

julia> d = Dict(1=>2)
Dict{Int64,Int64} with 1 entry:
  1 => 2

julia> keytype(d), keytype(typeof(d))
(Int64, Int64)

julia> a = [1 2]
1×2 Array{Int64,2}:
 1  2

julia> keytype(a), keytype(typeof(a))
ERROR: MethodError: no method matching keytype(::Type{Array{Int64,2}})
Closest candidates are:
  keytype(::AbstractArray{T,1} where T) at abstractarray.jl:100
  keytype(::AbstractArray) at abstractarray.jl:99
  keytype(::Type{#s56} where #s56<:AbstractDict{K,V}) where {K, V} at abstractdict.jl:237
  ...
Stacktrace:
 [1] top-level scope at none:0

julia> keytype(a)
CartesianIndex{2}

Thoughts?

Add keytype method for AbstractArray and AbstractVector (part 2)
Now also for the type, not just for objects.
@andyferris

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2018

I feel the fallback can be allowed to have allocations, whether or not the compler can elide them. If we have specific performance problems with Base types we can simply add some specializations.

And we'd need to implement it case-by-case for keytype on the type anyway, as we can't call keys on the type.

This is more convincing.

tkluck added 2 commits Mar 25, 2019
Merge remote-tracking branch 'origin/master' into keytype-abstractarray
Merge back master branch to validate clean merge and to kick off
an up-to-date CI build.
Add valtype(...) for arrays
Arrays are associative structures in the sense that keys(...)
and values(...) both work on them. When considering them as such, their
valtype should be equal to eltype. (A future version of Julia could
even do `const valtype = eltype`?)
@tkluck

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

Reviving this pull request as I recently found myself reaching for this. Merged back master (still a clean merge) and added valtype.

@tkluck tkluck changed the title Add keytype method for AbstractArray and AbstractVector Add `keytype`+`valtype` method for AbstractArray and AbstractVector Mar 25, 2019

base/abstractarray.jl Outdated Show resolved Hide resolved
@mbauman
Copy link
Member

left a comment

Let's make the type-based methods the "canonical" endpoints so a custom type would only need to override that one (instead of both).

base/abstractarray.jl Outdated Show resolved Hide resolved
base/abstractarray.jl Outdated Show resolved Hide resolved
base/abstractarray.jl Outdated Show resolved Hide resolved
mbauman and others added 4 commits Mar 27, 2019
Implement keytype/valtype for object as keytype(typeof(...
As suggested by @mbauman in code review.

Co-Authored-By: tkluck <tkluck@infty.nl>
@tkluck

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

All feedback is now incorporated, including the needs-* labels. The CI failures seem to be about dependency artifacts and not related to this PR.

@tkluck

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

Oooh let's get this in 1.2: https://discourse.julialang.org/t/julia-1-2-feature-freeze-april-4th/22478

@mbauman or @andyferris , do either of you have commit rights? Or should we bring in someone else?

@StefanKarpinski

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

It appears that this no longer needs NEWS or tests but still needs docs with compatibility annotation indicating that it was added in version 1.2. If @mbauman approves, we can merge. It would be great to get the docs with this PR but if time is an issue if you promise to add docs after the feature freeze...

@mbauman
Copy link
Member

left a comment

Thanks @tkluck! This looks great to me now. Let's merge once tests pass.

@mbauman mbauman added the arrays label Mar 29, 2019

@tkluck

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

@StefanKarpinski good catch, apologies. (FWIW, I (mis)understood that that label was related to the Compat.jl package, and that it would need the specific commit that introduced this feature -- i.e. that it wasn't actionable until this was merged.)

Just pushed those docs with a compatibility note.

base/abstractarray.jl Outdated Show resolved Hide resolved
base/abstractarray.jl Outdated Show resolved Hide resolved
@StefanKarpinski

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

Looks great! I made a suggestion to change the valtype example to use a string array since it seemed more obviously different than the types of the keys of an array which could be Int32 on some systems.

keytype/valtype: use String values in examples
this makes the distinction between key and value clearer. Thanks to @StefanKarpinski for suggesting this.

Co-Authored-By: tkluck <tkluck@infty.nl>
@tkluck

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

Travis uncovers failing

keytype_is_correct(Int64(1):Int64(5))

on 32-bits architecture. I'll check.

tkluck added 2 commits Mar 30, 2019
keytype/valtype for arrays: no special treatment for ranges
This was a little red herring in the discussion in #27749:
the `length` of a range may depend on the type of its parameters,
but as it turns out, `eltype(keys(...))` does not.

That's arguably a bug/inconsistency, but it's outside of the
scope of this pull request to fix that.
@tkluck

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2019

buildbot is happy now, including the new test cases introduced by this pull request. Appveyor and travis complain about unrelated things and are broken on master too.

I say, good to go :)

@StefanKarpinski
Copy link
Member

left a comment

Good to go as soon as CI passes. Please squash when merging!

@mbauman mbauman merged commit ce17e05 into JuliaLang:master Apr 4, 2019

12 of 14 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
buildbot/package_freebsd64 Run complete
Details
buildbot/package_linux32 Run complete
Details
buildbot/package_linux64 Run complete
Details
buildbot/package_macos64 Run complete
Details
buildbot/package_win32 Run complete
Details
buildbot/package_win64 Run complete
Details
buildbot/tester_freebsd64 Run complete
Details
buildbot/tester_linux32 Run complete
Details
buildbot/tester_linux64 Run complete
Details
buildbot/tester_macos64 Run complete
Details
buildbot/tester_win32 Run complete
Details
buildbot/tester_win64 Run complete
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.