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

Fix #394 with the missing keys after sorting #395

Merged
merged 1 commit into from
Jul 9, 2018
Merged

Fix #394 with the missing keys after sorting #395

merged 1 commit into from
Jul 9, 2018

Conversation

oxinabox
Copy link
Member

@oxinabox oxinabox commented Jul 9, 2018

This is my attempt at fixing #394.
Using idx = ht_keyindex2(d, key) or idx = ht_keyindex(d, key, true)
fixes the problem with the key vanishing,
but causes problems with out of bounds errors.

I tried a few other things.
Then I took a look at how rehash! worked,
and saw that it rebuilds the slots based only keys and values (if nothing has been deleted since last rehash!)
And so I figured: Just correct the keys and values and then rehash it.

I have also created a Julia0.6 branch with this change.
https://github.com/JuliaCollections/DataStructures.jl/tree/julia0.6
What I should have done is created a 0.6 branch without this change then PRed that.
So if this PR gets rejected cos it is wrong, that branch should be deleted.
Otherwise, that branch should be used to tag v0.8.4

@timholy this does not need to be ported to OrderedCollections.jl (I tried without looking by just stash applying the change set, and was informed that) OrderedCollections.jl is missing sorting functionality and that corresponding file.
I think that file should be moved over.
But that can be done with this change applied.

@codecov
Copy link

codecov bot commented Jul 9, 2018

Codecov Report

Merging #395 into master will decrease coverage by 0.34%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #395      +/-   ##
==========================================
- Coverage   98.81%   98.47%   -0.35%     
==========================================
  Files          30       30              
  Lines        1777     1771       -6     
==========================================
- Hits         1756     1744      -12     
- Misses         21       27       +6
Impacted Files Coverage Δ
src/dict_sorting.jl 90.9% <100%> (-1.4%) ⬇️
src/sorted_dict.jl 98.16% <0%> (-1.84%) ⬇️
src/ordered_dict.jl 92.48% <0%> (-1.84%) ⬇️
src/priorityqueue.jl 98.96% <0%> (-1.04%) ⬇️
src/deque.jl 98.23% <0%> (-0.07%) ⬇️
src/circ_deque.jl 100% <0%> (ø) ⬆️
src/sorted_multi_dict.jl 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 437f0bf...ebffdd1. Read the comment docs.

@StephenVavasis
Copy link
Contributor

I'll just remark that I proposed in #382 and #386 a complete rewrite of OrderedSet and OrderedDict in terms of Array and Dict (rather than in terms of low-level Julia primitives as in the current code). The purpose of the rewrite is to improve maintainability. The rewrite removes some undocumented functionality, namely, use of integer subscripts on OrderedSet, which appeared to be incompletely implemented anyway.

@oxinabox
Copy link
Member Author

oxinabox commented Jul 9, 2018

@StephenVavasis i do not thing this is mutually exclusive with that.
Either here, or in OrderedCollections.jl

In that this is (I think) simpler than that,
and so can be merged first while you are sorting out the re-basing (and maybe the rebalancing )you mentioned in the last comment.
And then overwritten with your change once that is ready to go.
(While keeping the new test)
If that is the way we are going to go with this.

However::
I recall that I didn't venue to say anything on that thread because I concluded that I had only a limitted understanding of how HashTables worked,
and even less how your SortedDicts etc worked.

As such I didn't venture and opinion there, and I still don't.
Honestly my understanding of if this fix even works is limited, and kind of trial and error.
It passes all the tests.

@timholy
Copy link
Member

timholy commented Jul 9, 2018

The rewrite removes some undocumented functionality, namely, use of integer subscripts on OrderedSet, which appeared to be incompletely implemented anyway.

I've already deprecated it in OrderedCollections, see JuliaCollections/OrderedCollections.jl@35ac721.

@timholy timholy merged commit 431e99d into master Jul 9, 2018
@ararslan ararslan deleted the ox/394 branch July 17, 2018 19:10
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.

3 participants