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 typing in Binary Heap #263

Merged
merged 1 commit into from
Jan 28, 2017
Merged

Conversation

ChrisRackauckas
Copy link
Contributor

The valtree in the binary heap is not strictly typed. In some codes I have, this causes a 2x performance reduction in the entire code (<.1% is data structure related) due to checking if a binary heap is empty. The simple fix is to make it Vector{T} instead of the non-concerete Array{T}.

Reference for performance problems: SciML/StochasticDiffEq.jl#13

@codecov-io
Copy link

codecov-io commented Jan 28, 2017

Current coverage is 95.85% (diff: 100%)

Merging #263 into master will not change coverage

@@             master       #263   diff @@
==========================================
  Files            30         30          
  Lines          2220       2220          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           2128       2128          
  Misses           92         92          
  Partials          0          0          

Powered by Codecov. Last update 52b74de...000fcbc

@kmsquire kmsquire merged commit 16f4450 into JuliaCollections:master Jan 28, 2017
@kmsquire
Copy link
Member

Thanks!

@ChrisRackauckas ChrisRackauckas deleted the PR-Heap branch January 28, 2017 15:47
@ChrisRackauckas
Copy link
Contributor Author

Could a patch be tagged with this change? This'll have a large effect on some of the main DifferentialEquations.jl methods. It's blocking a release which inlines a few more things which previously needed a function barrier to stop the propagation of this type instability.

@ChrisRackauckas
Copy link
Contributor Author

Though I understand if you want to wait until after you migrate to JuliaContainers

@kmsquire
Copy link
Member

I'll try to tag this today. The move to JuliaContainers shouldn't affect tagging, as a redirect will be in place.

@ChrisRackauckas
Copy link
Contributor Author

Thanks!

@kmsquire
Copy link
Member

@ChrisRackauckas, wondering if this is urgent or can wait a few days to get some additional changes in?

Reasoning isn't JuliaContainers (or whatever org that becomes), but more that there are changes in SortedDict constructors that warrant a minor version bump, as well as some upcoming changes in SortedSets and SortedMultiDicts (with pull requests) that warrant a minor version bump, and I would rather only bump the minor version once.

If this is urgent, I can just this change on a branch without the SortedDict changes and tag (cc: @tkelman), and then tag all of the sorted container changes at once (this commit will still be in the history, albeit with a different SHA).

Let me know your thoughts.

@ChrisRackauckas
Copy link
Contributor Author

It's not urgent. If it takes a week or two I wouldn't mind. I mentioned it because the tag frequency in this repo looks unpredictable, and that is a small enough change that I was hoping to make sure that it wouldn't just sit two months waiting for a larger change to happen. But since it sounds like you have something else on your plate that you're churning through, I can wait.

@kmsquire
Copy link
Member

kmsquire commented Jan 29, 2017 via email

@ChrisRackauckas
Copy link
Contributor Author

Thanks!

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