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 a DataStructures module with PriorityQueue and heap functions. #2920

Merged
merged 6 commits into from
Apr 30, 2013

Conversation

dcjones
Copy link
Contributor

@dcjones dcjones commented Apr 23, 2013

This is good to go, and everyone seems cool with it. See #2438.

@ViralBShah
Copy link
Member

I believe the module name Datastructures will conflict with the existing Datastructures.jl package.

Cc: @lindahua

@ViralBShah
Copy link
Member

An alternative name could be Collections.

@ViralBShah
Copy link
Member

It would be great if docs can be added before merging this.

@dcjones
Copy link
Contributor Author

dcjones commented Apr 24, 2013

Now that you mention it, I think I prefer Collections to DataStructures. One word is better than two.

I've renamed and documented everything now.

@ViralBShah
Copy link
Member

exports.jl still seems to be exporting DataStructures.

@dcjones
Copy link
Contributor Author

dcjones commented Apr 24, 2013

Oops. FIxed now.

end


end # module DataStructures
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Silly - but I guess this should be Collections too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly. Fixed now.

@ViralBShah
Copy link
Member

Thanks. I am going to leave this open for any other API discussions for a little bit.

ViralBShah added a commit that referenced this pull request Apr 24, 2013
Heaps are added in #2920
Graphs.jl has sophisticated graph functionality.
Remove examples/RMT now that we have the RandomMatrices.jl package
@stevengj
Copy link
Member

Why not just have this in Base (as opposed to Base.Collections or whatever submodule)?

@dcjones
Copy link
Contributor Author

dcjones commented Apr 25, 2013

There's a larger question of how Base should be organized, but with the addition of the Test and Sort modules we seem to be headed in the direction of not putting everything at top-level, so I'm following that trend.

Also, I expect there to be more in the Collections module eventually.

@lindahua
Copy link
Contributor

At line 126 of collections.jl, index should be declared as Dict{K, Int} instead of Dict. I have experienced this many times: if the field types are not completely specific, you will see huge performance penalty. I sometimes see nearly 100x slow down if such a field is at the critical path.

I think a careful benchmark is needed to ensure that the implementation is optimized.

@ViralBShah
Copy link
Member

BTW, API-wise this looks good. I think it should be merged, but would like @StefanKarpinski and @JeffBezanson to look at this as well.

@lindahua Agree that we should benchmark this. That can be done even after this pull request is merged though. It would be great if Base.Test can have some support for performance benchmarking, coming to think of it. More generally, we need a way to track performance regressions.

@JeffBezanson
Copy link
Member

Please rebase, then I will merge it.

@dcjones
Copy link
Contributor Author

dcjones commented Apr 30, 2013

Rebased!

@pao
Copy link
Member

pao commented Apr 30, 2013

Priority queue tests do not pass: https://travis-ci.org/JuliaLang/julia/jobs/6747796#L602

Appears to be related to the changes in #2347.

@dcjones
Copy link
Contributor Author

dcjones commented Apr 30, 2013

Thanks. It should be working now.

ViralBShah added a commit that referenced this pull request Apr 30, 2013
Add a DataStructures module with PriorityQueue and heap functions.
@ViralBShah ViralBShah merged commit bbc1557 into master Apr 30, 2013
@ViralBShah ViralBShah mentioned this pull request May 7, 2013
@StefanKarpinski StefanKarpinski deleted the dcj/pq2 branch November 16, 2013 16:44
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.

8 participants