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

Move PriorityQueue over from Base #238

Merged
merged 62 commits into from
Jan 5, 2017
Merged

Move PriorityQueue over from Base #238

merged 62 commits into from
Jan 5, 2017

Conversation

ararslan
Copy link
Member

Ref JuliaLang/julia#18497

This PR copies the definitions, tests, and documentation from Base.Collections.PriorityQueue into DataStructures in preparation for their imminent removal from Base.

cc @StefanKarpinski

@codecov-io
Copy link

codecov-io commented Dec 16, 2016

Current coverage is 95.76% (diff: 87.80%)

Merging #238 into master will decrease coverage by 0.47%

@@             master       #238   diff @@
==========================================
  Files            28         30     +2   
  Lines          2051       2174   +123   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1974       2082   +108   
- Misses           77         92    +15   
  Partials          0          0          

Powered by Codecov. Last update 830413f...4db0ec5

@ararslan
Copy link
Member Author

This works on 0.5 and nightly but not on 0.4. Any guidance for how to get this working on 0.4 would be much appreciated.

@kmsquire
Copy link
Member

One small issue (present elsewhere DayaStructures.jl) is that we conflate basic data structures (heaps) with abstract data types (priority queues). It would be nice to disentangle these.

Any chance to share code here with Heaps?

@ararslan
Copy link
Member Author

I'm sure that's possible but this is well outside my realm of knowledge. Perhaps this change is better left for someone who's more familiar with these things to take on.

@vtjnash
Copy link
Contributor

vtjnash commented Dec 22, 2016

should open / move JuliaLang/julia#4372 here when this merges

ararslan and others added 25 commits December 31, 2016 12:33
Add a DataStructures module with PriorityQueue and heap functions.
…is eliminates many definitions and doesn't change typical uses at all
improved error messages, removed function names from error text; no changes to error formatting.
Fix PriorityQueue(ks, vs) constructor
WIP: redesign of tuples and tuple types
return pairs instead of tuples when iterating Associatives
Remove last rst docstrings
Style: no empty newlines at start or end of functions
…/base/ (#16498)

* Replace deprecated Array(T..., dims...) with Array{T...}(dims...) for arrays.

* Replace deprecated Array(T..., dims...) with Array{T...}(dims...) in base/pkg/.

* Replace deprecated Array(T..., dims...) with Array{T...}(dims...) in base/linalg/.

* Replace deprecated Array(T..., dims...) with Array{T...}(dims...) in base/strings/.

* Replace deprecated Array(T..., dims...) with Array{T...}(dims...) in base/unicode/.

* Replace deprecated Array(T..., dims...) with Array{T...}(dims...) in base/grisu/.

* Replace deprecated Array(T..., dims...) with Array{T...}(dims...) in base/fft/.

* Replace deprecated Array(T..., dims...) with Array{T...}(dims...) in base/ for files starting b-e.

* Replace deprecated Array(T..., dims...) with Array{T...}(dims...) in base/docs/.

* Replace deprecated Array(T..., dims...) with Array{T...}(dims...) in base/ for files starting with f-i.

* Replace deprecated Array(T..., dims...) with Array{T...}(dims...) in base/dSFMT.jl, base/Enums.jl, and base/LineEdit.jl.

* Replace deprecated Array(T..., dims...) with Array{T...}(dims...) in base/ for files starting with l-m.

* Replace deprecated Array(T..., dims...) with Array{T...}(dims...) in base/ for files starting with p-r.

* Replace deprecated Array(T..., dims...) with Array{T...}(dims...) in base/ for files starting with s-t.

* Replace deprecated Array(T..., dims...) with Array{T...}(dims...) in base/special/.

* Additional changes from Array(T, dim) to Array{T}(dim).

* Further changes.

* fix constructors.rst
fix and consolidate uses of copy(a) and copy(similar(a),a) in to copymutable(a)
MORE doc improvements for arrays, and for eigen
Add a DataStructures module with PriorityQueue and heap functions.
@ararslan
Copy link
Member Author

Okay, I've preserved all authorship information for the moved code, which is why this suddenly looks like an enormous mess.

I've preserved the code from Base which provides heap operations on arrays for convenience (other packages, e.g. QuadGK, use that). I hope that's okay.

@ararslan
Copy link
Member Author

Is there any chance the primary maintainers here would be interested in dropping Julia 0.4 support to accommodate this change? It uses copymutable which doesn't appear to be a thing in 0.4, and IIRC it was failing on 0.4 even without that.

@StephenVavasis
Copy link
Contributor

I would be against dropping 0.4 support at this time. My own code has encountered a bug in 0.5.0 (significant performance loss for arrays with 6 or more subscripts). This bug is patched in 0.6.0-dev, but the patch has not yet been backported. So I am still using 0.4. I suspect I am not alone in this regard.

@ararslan
Copy link
Member Author

Do you have any suggestions for how to make the code in this PR work on 0.4 then, @StephenVavasis?

@StephenVavasis
Copy link
Contributor

What does copymutable do? I didn't find it in the 0.5.1 predocumentation. Would deepcopy be a reasonable substitute?

@ararslan
Copy link
Member Author

From the current master docstring:

Make a mutable copy of an array or iterable a. For a::Array, this is equivalent to copy(a), but for other array types it may differ depending on the type of similar(a). For generic iterables this is equivalent to collect(a).

I'm not really sure what the closest analog would be in 0.4, but I guess copy or deepcopy, since in here it's only being used on AbstractArrays?

@ararslan
Copy link
Member Author

Huh, nice. I guess that did it! This actually works on 0.4 now! Thanks so much, @StephenVavasis!

@ararslan
Copy link
Member Author

ararslan commented Dec 31, 2016

If this is good to go, can someone merge this and tag the package at their earliest convenience so that JuliaLang/julia#19800 can make it into Base before the 0.6 feature freeze? (which is tonight...)

@ararslan
Copy link
Member Author

ararslan commented Jan 3, 2017

As an aside, whoever merges this will probably want to ensure that they do not squash on merge, otherwise the history will not be preserved (AFAIK).

@tkelman
Copy link
Contributor

tkelman commented Jan 4, 2017

Can we get this in please? Even if it doesn't fit perfectly with the rest of the package's functionality, this is a much better place for it and it can be modified going forward.

@tkelman
Copy link
Contributor

tkelman commented Jan 5, 2017

I don't want to step on any toes of the people who do most of the maintenance work on this package, but we're trying to get base feature frozen for 0.6 so in a hurry here. If I don't hear a strong objection I'm likely to merge this.

@tkelman tkelman merged commit 3afb7a2 into JuliaCollections:master Jan 5, 2017
@ararslan ararslan deleted the aa/priorityqueue branch January 5, 2017 17:22
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.