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

filter(f, ::Dict) inconsistency #17886

Closed
TotalVerb opened this issue Aug 8, 2016 · 3 comments
Closed

filter(f, ::Dict) inconsistency #17886

TotalVerb opened this issue Aug 8, 2016 · 3 comments
Assignees
Labels
collections Data structures holding multiple items, e.g. sets
Milestone

Comments

@TotalVerb
Copy link
Contributor

TotalVerb commented Aug 8, 2016

filter is inconsistent with its cousin map:

julia> map(x -> last(x) => first(x), Dict(1 => 2, 3 => 1))
Dict{Int64,Int64} with 2 entries:
  2 => 1
  1 => 3

julia> map((x, y) -> y => x, Dict(1 => 2, 3 => 1))
ERROR: MethodError: no method matching (::##31#32)(::Pair{Int64,Int64})
Closest candidates are:
  #31(::Any, ::Any)
 in _collect(::Dict{Int64,Int64}, ::Base.Generator{Dict{Int64,Int64},##31#32}, ::Base.EltypeUnknown, ::Base.HasLength) at ./array.jl:284
 in map(::Function, ::Dict{Int64,Int64}) at ./abstractarray.jl:1427

but

julia> filter(x -> last(x) > first(x), Dict(1 => 2, 3 => 1))
ERROR: MethodError: no method matching (::##33#34)(::Int64, ::Int64)
Closest candidates are:
  #33(::Any)
 in filter(::##33#34, ::Dict{Int64,Int64}) at ./dict.jl:179

julia> filter((x, y) -> y > x, Dict(1 => 2, 3 => 1))
Dict{Int64,Int64} with 1 entry:
  1 => 2

I think the map behaviour makes more sense and is more consistent with the rest of the language.

To summarize:

Function array string dict ImmutableDict
map one-arg one-arg one-arg not supported
filter one-arg one-arg two-arg ?? two-arg ??
@TotalVerb TotalVerb changed the title filter(f, Dict) inconsistency filter(f, ::Dict) inconsistency Aug 8, 2016
@TotalVerb
Copy link
Contributor Author

I understand there is a convenience difference here, but that has been alleviated with the new guard comprehension syntax. Now there's

Dict(k => v for (k, v) in d if k < v)

So the convenience argument is nullified somewhat. The argument in favour of one-arg is less surprise, and easier generic programming on iterables.

@garborg
Copy link
Contributor

garborg commented Aug 12, 2017

I just ran into the same inconsistency, this time against Iterators.filter:

   _       _ _(_)_     |  A fresh approach to technical computing
  (_)     | (_) (_)    |  Documentation: https://docs.julialang.org
   _ _   _| |_  __ _   |  Type "?help" for help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 0.7.0-DEV.1246 (2017-08-05 10:31 UTC)
 _/ |\__'_|_|_|\__'_|  |  Commit 472353c089 (6 days old master)
|__/                   |  x86_64-linux-gnu

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

julia> first(filter((kv) -> true, d))
ERROR: MethodError: no method matching (::##1#2)(::Int64, ::Int64)
Closest candidates are:
  #1(::Any) at REPL[2]:1
Stacktrace:
 [1] filter(::##1#2, ::Dict{Int64,Int64}) at ./associative.jl:361

julia> first(filter((k, v) -> true, d))
1 => 1

julia> first(Iterators.filter((kv) -> true, d))
1 => 1

julia> first(Iterators.filter((k, v) -> true, d))
ERROR: MethodError: no method matching (::##7#8)(::Pair{Int64,Int64})
Closest candidates are:
  #7(::Any, ::Any) at REPL[5]:1
Stacktrace:
 [1] start_filter at ./iterators.jl:286 [inlined]
 [2] start(::Base.Iterators.Filter{##7#8,Dict{Int64,Int64}}) at ./iterators.jl:281
 [3] first(::Base.Iterators.Filter{##7#8,Dict{Int64,Int64}}) at ./abstractarray.jl:148

@StefanKarpinski StefanKarpinski added this to the 1.0 milestone Aug 12, 2017
@StefanKarpinski
Copy link
Member

This inconsistency is part of the API consistency fixes that we need for 1.0. I believe @JeffBezanson has already thought and worked towards addressing this, but this issue is one specific problem.

@JeffBezanson JeffBezanson self-assigned this Aug 17, 2017
@JeffBezanson JeffBezanson added the collections Data structures holding multiple items, e.g. sets label Aug 17, 2017
JeffBezanson added a commit that referenced this issue Aug 19, 2017
fix #17886, `filter[!]` on dicts should pass 1 argument to the function
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
collections Data structures holding multiple items, e.g. sets
Projects
None yet
Development

No branches or pull requests

4 participants