-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
RFC: OrderedDict redux #4038
RFC: OrderedDict redux #4038
Conversation
Performance numbers below, after warmup. Interestingly, Before:
test_ins
========
Dict{K,V}: median=0.28366528200000013
test_del
========
Dict{K,V}: median=0.4370309559999964
test_ins_del
============
Dict{K,V}: median=0.49659235499999754
===============================================================================================================
With this patch:
Nothing = Unordered,
Int64 = Ordered
test_ins
========
Dict{String,Int,Nothing}: median=0.2622292050000018
Dict{String,Int,Int64}: , median=0.31364540300000016
test_del
========
Dict{String,Int,Nothing}: median=0.3849849159999983
Dict{String,Int,Int64}: median=0.4319687069999986
test_ins_del
============
Dict{String,Int,Nothing}: median=0.4332843499999996
Dict{String,Int,Int64}: median=0.47339647700000054
Test code is here: https://gist.github.com/kmsquire/6217215 |
Never mind, hadn't yet seen that this was part of a pull-request that explains what you're doing. |
Very interesting. This leverages the fact that an |
I've been sitting on this for a while, and wanted to get it out. If it weren't for the extra type parameter, this would probably be a no-brainer--doesn't reduce performance, and adds useful functionality. The Ordered/Unordered type parameter makes it a little clunky, so I'm hoping there are ways to make it nicer (such as default type parameter values). |
It would be nice to get this added somewhere, but I wonder if it doesn't belong in DataStructures rather than Base. |
That would be fine. The main annoyance here is that the trivial implementation of |
That's a fair point. Maybe it should just be in Base then. |
If I can remove the main annoyance of the current patch, which is that |
Right, it turns out that I looked at that before. For a type with 3 type parameters, there does not seem to be any way currently to call the type constructor by only specifying two. |
More details: If I change the basic type constructor to this: type Dict{K,V,O} <: Associative{K,V}
...
function Dict()
if !(O <: Union(Unordered, Ordered))
return Dict{K,V,Unordered}()
end
n = 16
new(zeros(Uint8,n), Array(K,n), Array(V,n), Array(O,n), Array(O,0), 0, 0, identity)
end
...
end Then this works: julia> Dict{String,Int,Any}()
Dict{String,Int64,Nothing}() But this doesn't: julia> Dict{String,Int}()
ERROR: type cannot be constructed |
This is yet another redo of OrderedDict.
Unlike previous versions, this version grafts ordering onto the current Dict infrastructure directly, with no impact to the performance of unordered dicts.
Features:
Unordered
is a typealias for NothingOrdered
is a typealias for IntDict
functions, reusingDict
code where possible.OrderedDict{K,V}()
type constructor returns aDict{K,V,Ordered}
OrderedDict{K,V}()
, and also haveOrderedDict()
outer constructorsOrderedSet{K}
(K=>V)[]
creates aDict{K,V,Unordered}
Downsides:
Ordered
orUnordered
when calling the mainDict
orSet
constructor explicitlyComments:
Unordered
as a type parameter to theDict
/Set
constructors is annoying. For Dict,(K=>V)
is a good alternative; there's nothing like that forSet
UnorderedDict()
orUnorderedSet
, which is the same amount of typing.{}
forAny[]
, and reserve{}
for Set notation?Base.Order.Reverse
can returnOrder.Forward
(but only under rare circumstances)I'm actually less concerned about the type abuse than the extra type parameter to
Dict
/Set
.Any and all thoughts/suggestions/comments welcome.