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 indexing to OrderedSets #180

Merged
merged 1 commit into from Feb 9, 2016
Merged

Add indexing to OrderedSets #180

merged 1 commit into from Feb 9, 2016

Conversation

ahwillia
Copy link
Contributor

@ahwillia ahwillia commented Feb 8, 2016

Discussed here: #178

@ahwillia
Copy link
Contributor Author

ahwillia commented Feb 8, 2016

I was using an outdated version, so the code is tweaked slightly. Just by providing indexing we can rely on the findfirst and findin functions provided in Base to find element indices...

@kmsquire
Copy link
Member

kmsquire commented Feb 9, 2016

LGTM. @DanielArndt @rawls238, any thoughts? If you have nothing against this, I think it can be merged.

@rawls238
Copy link
Contributor

rawls238 commented Feb 9, 2016

this lgtm as well

rawls238 added a commit that referenced this pull request Feb 9, 2016
Add indexing to OrderedSets
@rawls238 rawls238 merged commit 9d98404 into JuliaCollections:master Feb 9, 2016
@timholy
Copy link
Member

timholy commented Jun 26, 2018

As discovered by @StephenVavasis, indexing does not currently play well with deletion:

julia> function testindexing()
           s = OrderedSet([i for i = 1:12])
           delete!(s, 7)
           prerh = s[7]
           OrderedCollections.rehash!(s.dict)     # see #392 for the OrderedCollections name
           postrh = s[7]
           return (prerh, postrh)
       end

julia> testindexing()
(7, 8)

(I did this inside a function because simply showing at the REPL rehashes the dictionary, so it's easy for this problem to be disguised.)

Our options appear to be to check the dirty flag before every indexing operation, or just drop indexing altogether. It's worth noting that one can naturally rehash! once when you first begin iteration (and hence is efficient), but indexing requires a check each time. It is just a Bool check, so perhaps not a big deal, but since it's a mutable struct it's not entirely free.

So, worth asking, how useful is this really? If we do drop it, I can add a deprecation that also has the proper behavior.

@yakir12
Copy link

yakir12 commented Apr 19, 2019

Sorry for butting in, but if we shouldn't index but use iteration instead, why not add a function like this:

function getindex(s, i)
    for (j, sj) in enumerate(s)
        j == i && return sj
    end
    throw(BoundsError(s, i))
end

@bzinberg
Copy link

bzinberg commented Jul 9, 2020

I've found myself often wanting an ordered dict object that is ergonomic to index into either by dict key or by index in the key sequence. And, often I don't need the ability to delete from this data structure. For this case I've created AppendOnlyOrderedDict which is basically a dict and a vector (which records the key order) glued together plus some syntactic sugar for the second kind of indexing.

>>> d = AppendOnlyOrderedDict(:a => 2, :b => 4, :c => 6);
>>> d[:b]
4
>>> d.seq[2]
:b => 4
>>> d.seqvals[2]
4
>>> d.seqvals[2:end]
2-element Array{Int64,1}:
 4
 6

ilia-kats added a commit to scverse/Muon.jl that referenced this pull request Apr 1, 2021
This is currently very slow. If adata has N names and we want to extract
K names, the complexity is O(NK). Ideally we would use an ordered set
for row_names and var_names, but we need the ordered set to support both
key and index lookup as well as being able to return an index for a
given key. OrderedSet from OrderedCollections.jl deprecated index lookup
and does not have an API for looking up an index for a given key (see
JuliaCollections/OrderedCollections.jl#64 and
JuliaCollections/DataStructures.jl#180 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants