Rebase on OrderedCollections #448

timholy · 2018-09-19T10:19:22Z

This removes the OrderedDict/OrderedSet functionality in favor of the implementation in OrderedCollections. Sorry this took so long.

oxinabox · 2018-09-19T10:21:17Z

Project.toml

@@ -4,6 +4,7 @@ version = "0.12.0"

 [deps]
 InteractiveUtils = "b77e0a4c-d291-57a0-90e8-8db25a27a240"
+OrderedCollections = "bac558e1-5e72-5ebc-8fee-abe8a469f55d"



Do we want to set a [compat] here?
It might make things more orthogonal.
Or it might not

I don't think it's necessary, since the latest tagged version of OrderedCollections works here.

Since I was making other changes, I added the compat

oxinabox · 2018-09-19T10:27:04Z

src/dict_support.jl

-_tablesz(x::Integer) = x < 16 ? 16 : one(x)<<((sizeof(x)<<3)-leading_zeros(x-1))
-hashindex(key, sz) = (reinterpret(Int,(hash(key))) & (sz-1)) + 1
-
-function not_iterator_of_pairs(kv)


I am not thrilled at the idea of depending on importing this from
OrderedCollections,
because out other dicts need it,
and ideally we'ld be able to cut them out into their own packages,
which would also not depend on OrderedCollections

I see two alternative options.

just duplicate the code. I don't mind this, while it is not DRY, it does let us evolve the remainind dicts independently of OrderedCollections

make a micro package DataStructuresUtil.jl or some such, and have both import this.

Interestingly, I think OrderedCollections doesn't even use that function. So I can put it back here and delete it from there.

codecov · 2018-09-19T10:31:56Z

Codecov Report

Merging #448 into master will increase coverage by 0.65%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #448      +/-   ##
==========================================
+ Coverage   98.07%   98.73%   +0.65%     
==========================================
  Files          30       28       -2     
  Lines        1558     1342     -216     
==========================================
- Hits         1528     1325     -203     
+ Misses         30       17      -13

Impacted Files	Coverage Δ
src/dict_support.jl	`100% <ø> (ø)`	⬆️

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 bdd7aa0...1d50fce. Read the comment docs.

oxinabox · 2018-09-19T10:42:34Z

Checking others
is_ordered is a trait, (exists reL #237)
and it conceptually belongs to the OrderedCollection namespace

oxinabox · 2018-09-19T10:43:37Z

src/DataStructures.jl

-    using InteractiveUtils: methodswith
+    using OrderedCollections
+    using OrderedCollections: not_iterator_of_pairs, rehash!
+    import OrderedCollections: filter, filter!, isordered


filter and filter! both come from Base
Why are they being imported?

Well, they have to be imported because DataStructures extends them. I imported them from OrderedCollections instead of Base due to the Iterators.filter separation---I basically thought "this should be the same filter that OrderedCollections uses, whatever version of filter that is."

oxinabox · 2018-09-19T10:44:34Z

src/DataStructures.jl

@@ -16,7 +16,9 @@ module DataStructures
                 eachindex, keytype, valtype
    import Base: iterate

-    using InteractiveUtils: methodswith
+    using OrderedCollections
+    using OrderedCollections: not_iterator_of_pairs, rehash!


rehash! AFAICT is only used by OrderedCollections
so I don't think we need it to be used.
(or did I miss an usage?)

DataStructures.jl/src/dict_sorting.jl

Line 6 in bdd7aa0

rehash!(d)

and

DataStructures.jl/src/dict_sorting.jl

Line 16 in bdd7aa0

rehash!(d)

Ah,
I thought that file had been moved over.
I think it should be moved over.
It uses only Base and OrderedCollections types, no?

Yes, you're absolutely right.

Oddly, these comments only show up if I access this page in certain ways. Weird GitHub bug?

oxinabox · 2018-09-19T10:46:34Z

Ok, I am done with my look over.
Thanks for doing this.

(Complete sidenote; thanks for the recommendation of Gopen and Swan's "The Science of Scientific Writing". I wish I'ld read it at the start of my PhD, but reading it before writing my thesis intro/conclusion is definitely better than never.)

timholy · 2018-09-19T13:46:10Z

Good to go? EDIT: nvm, just noticed your comment above.

Closes #392

timholy · 2018-09-20T09:07:57Z

REQUIRE

@@ -1 +1,2 @@
 julia 0.7
+OrderedCollections 1.0.1


Can we just get rid of this file now?

Nope, I checked this morning.
Attobot won't talk to you if you don't have a REQUIRE file.

timholy · 2018-09-20T10:47:00Z

Good to go? The appveyor failure seems independent.

timholy · 2018-09-20T21:41:05Z

Unless there are objections I'm going to merge this soon because in the current situation we are causing problems: see JuliaCollections/OrderedCollections.jl#10

oxinabox

LGTM

timholy · 2018-09-21T10:05:26Z

Thanks! Sorry this took more review from you than it should have.

oxinabox self-assigned this Sep 19, 2018

oxinabox reviewed Sep 19, 2018

View reviewed changes

timholy force-pushed the teh/fork_orderedcollections branch 3 times, most recently from a573b51 to 64807f6 Compare September 19, 2018 13:17

StephenVavasis mentioned this pull request Sep 19, 2018

Replaceordereddict #386

Closed

timholy added 2 commits September 20, 2018 03:55

Use OrderedCollections

fcc6d17

Closes #392

Remove OrderedCollections tests

1d50fce

timholy force-pushed the teh/fork_orderedcollections branch from 64807f6 to 1d50fce Compare September 20, 2018 08:55

timholy commented Sep 20, 2018

View reviewed changes

timholy mentioned this pull request Sep 20, 2018

Type piracy causing warnings JuliaCollections/OrderedCollections.jl#10

Closed

oxinabox approved these changes Sep 21, 2018

View reviewed changes

timholy merged commit 525f990 into master Sep 21, 2018

timholy deleted the teh/fork_orderedcollections branch September 21, 2018 10:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rebase on OrderedCollections #448

Rebase on OrderedCollections #448

timholy commented Sep 19, 2018

oxinabox Sep 19, 2018 •

edited

Loading

timholy Sep 19, 2018

timholy Sep 20, 2018

oxinabox Sep 19, 2018 •

edited

Loading

timholy Sep 19, 2018

codecov bot commented Sep 19, 2018 •

edited

Loading

oxinabox commented Sep 19, 2018

oxinabox Sep 19, 2018

timholy Sep 19, 2018

oxinabox Sep 19, 2018

timholy Sep 19, 2018

oxinabox Sep 19, 2018

timholy Sep 19, 2018

oxinabox commented Sep 19, 2018

timholy commented Sep 19, 2018 •

edited

Loading

timholy Sep 20, 2018

oxinabox Sep 20, 2018

timholy commented Sep 20, 2018

timholy commented Sep 20, 2018

oxinabox left a comment •

edited

Loading

timholy commented Sep 21, 2018

Rebase on OrderedCollections #448

Rebase on OrderedCollections #448

Conversation

timholy commented Sep 19, 2018

oxinabox Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxinabox Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 19, 2018 • edited Loading

Codecov Report

oxinabox commented Sep 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxinabox commented Sep 19, 2018

timholy commented Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timholy commented Sep 20, 2018

timholy commented Sep 20, 2018

oxinabox left a comment • edited Loading

Choose a reason for hiding this comment

timholy commented Sep 21, 2018

oxinabox Sep 19, 2018 •

edited

Loading

oxinabox Sep 19, 2018 •

edited

Loading

codecov bot commented Sep 19, 2018 •

edited

Loading

timholy commented Sep 19, 2018 •

edited

Loading

oxinabox left a comment •

edited

Loading