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

Revise workspace/buffer to be ::UInt8[] instead of ::AbstractVector{eltype(input)} #45596

Closed

Conversation

LilithHafner
Copy link
Member

The workspace/buffer used in sorting is merely a place to put things. It need not have a fixed type. I believe the most natural way to represent a place to put things is a Vector{UInt8}, as suggested originally by cjdoris. The impact of this change is perhaps best understood by seeing how the "sort(x; workspace=w)" test set changes.

To use a workspace/buffer, one needs to designate some space workspace = UInt8[] and then give that space to multiple different sorting functions, each of which will temporarily assume ownership of that Vector, resize/reinterpret it as needed, and then return ownership back to the caller upon return. To quote cjdoris, "The caller can then preallocate an empty such vector and pass it in to repeated calls without needing to know how it will be used."

This is better than workspace::AbstractVector{eltype(input)} because it is more natural, extensible, and reusable (subjective) and because it allows for the workspace to be used both to store elements of the input array and to store other relevant collections with different element types like the count vector in radix sort (objective).

See discussion leading to this PR here and more recently here. This follows up on #45330.

@LilithHafner
Copy link
Member Author

While I still think there is theoretical merit to this approach, I am giving up pursuant to this discussion.

The complexities are too large and the reward too small for me to do this without additional language support. I've folded separable parts of this PR into #45570.

LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Jun 8, 2022
…so minor style changes and fixups from JuliaLang#45596 and local review.
oscardssmith pushed a commit that referenced this pull request Jun 16, 2022
* Fix and test sort!(OffsetArray(rand(200), -10))

* Convert to 1-based indexing rather than generalize to arbitrary indexing

* avoid overhead of views where reasonable

* style

* handle edge cases better, making the workspace function unhelpful. Also minor style changes and fixups from #45596 and local review.

* move comments in tests for discoverability

Co-authored-by: Lilith Hafner <[email protected]>
@ViralBShah ViralBShah added the sorting Put things in order label Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sorting Put things in order
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants