-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 workspace/buffer support to sorting #45330
Conversation
… this feature in sort(A; dims)
I think SciML usually calls it |
I crossposted to discourse and am crossposting Jar1's response back
|
There's likely still room for improvement by letting the @btime sort(x; dims=1) setup=(x=rand(Int, 3000, 3000)) evals=1;
v1.7.2 => 357.704 ms (5 allocations: 68.66 MiB)
v1.9.0 => 10.980 s (12005 allocations: 201.24 GiB)
PR => 229.195 ms (3018 allocations: 150.60 MiB) |
Bump :) |
I would've preferred the keyword name Is the intention to keep this as an internal undocumented feature? |
Agree that |
Both seem fine to me. There is no reason we have to stick to "workspace" just because this merged. Googling definitions yields: Buffer: Workspace: Scratchspace: As long as we reach consensus by the time we start 1.9 prereleases this shouldn't be a problem. |
FWIW I just talked with a CS researcher who doesn't use Julia. She prefers "workspace". |
Another CS researcher who doesn't use Julia prefers Workspace over Cache, Buffer, and Scratchspace, saying "I feel like workspace is a more intuitive word for non-computer scientist people to understand, if you have to use something" |
I think that's arguable, but I thought I'd share my reaction looking through this PR. I'd hazard that throughout most of the Julia code base buffer is used to denote this concept. Workspace to me denotes something entirely different. For example, in MATLAB (https://www.mathworks.com/help/matlab/ref/workspace.html) and VS Code https://code.visualstudio.com/docs/editor/workspaces. This was why I was thrown off initially. Apparently, there is a precedent from Fortran to use workspace for the concept, but that almost feels to me a little antiquated. |
Unless anyone else chimes in in favor of keeping |
* 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]>
The renaming pull request is here |
As this is internal, I don't think it needs news or a compat annotation. |
This PR adds an additional workspace argument to most internal sorting functions and allows the end-user to pass one in with the keyword argument
workspace
(undocumented).There are two types of workspaces: nothing, and an AbstractVector. nothing indicates that sorting functions should allocate new workspaces as needed and discard them when done. An AbstractVector indicates that sorting functions should use that vector, resizing it larger if needed. Wherever possible, sorting functions can handle receiving nothing, and AbstractVector with appropriate eltype, or no argument at all.
This PR also utilizes that feature to implement solution 3 of #45326 and therefore closes #45326.
As is, this PR exposes a new public-facing keyword argument for (almost) all sorting functions:
Is there a standard name for this optional keyword argument?