Remove specialized sort! method for PartialQuickSort{Int} (fixes #12833) #12838
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There were two sort methods for PartialQuickSort, depending on whether it was parameterized by an Int (representing the last index needing to be sorted) or a Range. The version parameterized by an Int has one less comparison per branch, but it causes inference to fail to infer the return type of sort/sort! under many circumstances. Removing this version and only providing the generic function restores proper type inference. If there's an underlying bug that is fixed, this can be reverted (though the test should remain.)
|
I restarted the two travis jobs for you; both failed on x86_64. |
|
This fixes sbromberger/LightGraphs.jl#147. |
| ## has one less comparison per loop than the version below, but enabling | ||
| ## it causes return type inference to fail for sort/sort! (#12833) | ||
| ## | ||
| # function sort!(v::AbstractVector, lo::Int, hi::Int, a::PartialQuickSort{Int}, |
There was a problem hiding this comment.
Couldn't you do sort!{T}(v::AbstractVector{T}, ...) here, and then return v::AbstractVector{T} on line 369 to preserve type stability?
There was a problem hiding this comment.
This didn't fix the overallocations: #12833 (comment)
JeffBezanson
added a commit
that referenced
this pull request
Aug 28, 2015
Remove specialized sort! method for PartialQuickSort{Int} (fixes #12833)
|
@JeffBezanson, can you explain why this additional method caused inference to fail? Too many |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There were two
sortmethods forPartialQuickSort, dependingon whether it was parameterized by an
Int(representing thelast index needing to be sorted) or a
Range. The versionparameterized by an
Inthas one less comparison per branch,but it causes inference to fail to infer the return type of
sort/sort!under many circumstances. Removing this versionof
sort!and only providing the generic function restores propertype inference.
If there's an underlying bug that is fixed, this can be reverted
(although the test should remain.)