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

support sorting tuples #56425

Closed
wants to merge 1 commit into from
Closed

Conversation

nsajko
Copy link
Contributor

@nsajko nsajko commented Nov 3, 2024

Uses merge sort, as an obvious choice for a stable sort of tuples.

A recursive data structure of singleton type, representing Peano natural numbers, is used to help with splitting a tuple into two halves in the merge sort. An alternative design would use a reference tuple, but this would require relying on tail more, which seems more harsh on the compiler. With the recursive datastructure the predecessor operation and the successor operation are both trivial.

Allows inference to preserve inferred element type even when tuple length is not known.

Follow-up PRs may add further improvements, such as the ability to select an unstable sorting algorithm.

The added file, typedomainnumbers.jl is not specific to sorting, thus making it a separate file. Xref #55571.

Fixes #54489

@nsajko nsajko added feature Indicates new feature / enhancement requests sorting Put things in order labels Nov 3, 2024
@nsajko nsajko force-pushed the stable_tuple_sort branch from 6b10e70 to d573756 Compare November 3, 2024 00:55
@nsajko nsajko mentioned this pull request Nov 3, 2024
@nsajko nsajko force-pushed the stable_tuple_sort branch 6 times, most recently from 6f49f4c to 8d3fe04 Compare November 8, 2024 05:53
@KristofferC
Copy link
Member

The complexity of this code makes me kind of uncomfortable, especially the typed natural number stuff.

How about copy pasting the SortedNetwork implementations up to length 16 and then allocate above that?

@nsajko
Copy link
Contributor Author

nsajko commented Nov 8, 2024

the typed natural number stuff.

FTR the type domain numbers are applicable to other places, too, I'll be putting up more PRs relying on that code. Also, it's not like there's any complex logic there, it's mostly just unconvential, I think. That reminds me though, I didn't add any tests for type domain numbers here.

How about copy pasting the SortedNetwork implementations up to length 16

  1. That wouldn't be a stable sort. (As far as I remember, stable sorting networks are actually possible, but bigger/less efficient than unstable sorting networks.)
  2. The current code supports lengths up to 31. And arbitrary lengths in principle, with some adjustment.

@nsajko nsajko force-pushed the stable_tuple_sort branch from 8d3fe04 to 8e2b350 Compare November 8, 2024 08:03
@adienes
Copy link
Contributor

adienes commented Nov 8, 2024

it kind of feels like the sorting stuff here is just a facade to sneak in the (previously rejected) type domain number stuff

@nsajko
Copy link
Contributor Author

nsajko commented Nov 8, 2024

Are you trying to be inflammatory or something? Do you really have to do that here, in the PR discussion?

@adienes
Copy link
Contributor

adienes commented Nov 8, 2024

given that you closed #55571 with the comment

I guess I'll try making the next PR more minimal, in the sense that only methods that are necessary for a specific improvement somewhere in Base will be included, then if that PR gets merged I'll try following up.

I guess the adjective I would use is "observant"

@nsajko
Copy link
Contributor Author

nsajko commented Nov 8, 2024

I'll bite, what's your point?

@mcabbott
Copy link
Contributor

mcabbott commented Nov 8, 2024

I agree this seems like a lot of code, compared to these 20 lines which handle NTuples in #54494 . What exactly is gained, in what use cases? It seems you claim better inference, can you show examples where this matters?

How about copy pasting the SortedNetwork implementations up to length 16 and then allocate above that?

I assume this refers to https://github.com/JeffreySarnoff/SortingNetworks.jl , which writes out the steps for each length. That also seems attractively simple.

@nsajko
Copy link
Contributor Author

nsajko commented Nov 8, 2024

The benefit is that this supports Tuple, while that PR just supports NTuple.

I assume this refers to https://github.com/JeffreySarnoff/SortingNetworks.jl , which writes out the steps for each length. That also seems attractively simple.

As I said above, that can't be the default sort because it's not stable.

@nsajko nsajko force-pushed the stable_tuple_sort branch 2 times, most recently from c75f73d to 06a822d Compare November 9, 2024 00:02
@LilithHafner
Copy link
Member

The benefit is that this supports Tuple, while that PR just supports NTuple.

It would be trivial (though IMO a bad idea) to make #54494 support non-homogenous tuples.

@nsajko
Copy link
Contributor Author

nsajko commented Nov 9, 2024

It would be trivial (though IMO a bad idea) to make #54494 support non-homogenous tuples.

Of course, but not with good inference.

Uses merge sort, as an obvious choice for a stable sort of tuples.

A recursive data structure of singleton type, representing Peano
natural numbers, is used to help with splitting a tuple into two halves
in the merge sort. An alternative design would use a reference tuple,
but this would require relying on `tail`, which seems more harsh on the
compiler. With the recursive datastructure the predecessor operation
and the successor operation are both trivial.

Allows inference to preserve inferred element type even when tuple
length is not known.

Follow-up PRs may add further improvements, such as the ability to
select an unstable sorting algorithm.

The added file, typedomainnumbers.jl is not specific to sorting, thus
making it a separate file. Xref JuliaLang#55571.

Fixes JuliaLang#54489
nsajko added a commit to nsajko/julia that referenced this pull request Nov 10, 2024
This replaces `Fix` (xref JuliaLang#54653) with `fix`. The usage is similar: use
`fix(i)(f, x)` instead of `Fix{i}(f, x)`.

Benefits:
* Improved type safety: creating an invalid type such as
  `Fix{:some_symbol}` or `Fix{-7}` is not possible.
* The design should be friendlier to future extensions. E.g., suppose
  that publicly-facing functionality for fixing a keyword (instead of
  positional) argument was desired, it could be achieved by adding a
  new method to `fix` taking a `Symbol`, instead of adding new public
  names.

Lots of changes are shared with PR JuliaLang#56425, if one of them gets merged
the other will be greatly simplified.
nsajko added a commit to nsajko/julia that referenced this pull request Nov 10, 2024
This replaces `Fix` (xref JuliaLang#54653) with `fix`. The usage is similar: use
`fix(i)(f, x)` instead of `Fix{i}(f, x)`.

Benefits:
* Improved type safety: creating an invalid type such as
  `Fix{:some_symbol}` or `Fix{-7}` is not possible.
* The design should be friendlier to future extensions. E.g., suppose
  that publicly-facing functionality for fixing a keyword (instead of
  positional) argument was desired, it could be achieved by adding a
  new method to `fix` taking a `Symbol`, instead of adding new public
  names.

Lots of changes are shared with PR JuliaLang#56425, if one of them gets merged
the other will be greatly simplified.
nsajko added a commit to nsajko/julia that referenced this pull request Nov 10, 2024
This replaces `Fix` (xref JuliaLang#54653) with `fix`. The usage is similar: use
`fix(i)(f, x)` instead of `Fix{i}(f, x)`.

Benefits:
* Improved type safety: creating an invalid type such as
  `Fix{:some_symbol}` or `Fix{-7}` is not possible.
* The design should be friendlier to future extensions. E.g., suppose
  that publicly-facing functionality for fixing a keyword (instead of
  positional) argument was desired, it could be achieved by adding a
  new method to `fix` taking a `Symbol`, instead of adding new public
  names.

Lots of changes are shared with PR JuliaLang#56425, if one of them gets merged
the other will be greatly simplified.
nsajko added a commit to nsajko/julia that referenced this pull request Nov 10, 2024
This replaces `Fix` (xref JuliaLang#54653) with `fix`. The usage is similar: use
`fix(i)(f, x)` instead of `Fix{i}(f, x)`.

Benefits:
* Improved type safety: creating an invalid type such as
  `Fix{:some_symbol}` or `Fix{-7}` is not possible.
* The design should be friendlier to future extensions. E.g., suppose
  that publicly-facing functionality for fixing a keyword (instead of
  positional) argument was desired, it could be achieved by adding a
  new method to `fix` taking a `Symbol`, instead of adding new public
  names.

Lots of changes are shared with PR JuliaLang#56425, if one of them gets merged
the other will be greatly simplified.
nsajko added a commit to nsajko/julia that referenced this pull request Nov 10, 2024
This replaces `Fix` (xref JuliaLang#54653) with `fix`. The usage is similar: use
`fix(i)(f, x)` instead of `Fix{i}(f, x)`.

Benefits:
* Improved type safety: creating an invalid type such as
  `Fix{:some_symbol}` or `Fix{-7}` is not possible.
* The design should be friendlier to future extensions. E.g., suppose
  that publicly-facing functionality for fixing a keyword (instead of
  positional) argument was desired, it could be achieved by adding a
  new method to `fix` taking a `Symbol`, instead of adding new public
  names.

Lots of changes are shared with PR JuliaLang#56425, if one of them gets merged
the other will be greatly simplified.
nsajko added a commit to nsajko/julia that referenced this pull request Nov 10, 2024
This replaces `Fix` (xref JuliaLang#54653) with `fix`. The usage is similar: use
`fix(i)(f, x)` instead of `Fix{i}(f, x)`.

Benefits:
* Improved type safety: creating an invalid type such as
  `Fix{:some_symbol}` or `Fix{-7}` is not possible.
* The design should be friendlier to future extensions. E.g., suppose
  that publicly-facing functionality for fixing a keyword (instead of
  positional) argument was desired, it could be achieved by adding a
  new method to `fix` taking a `Symbol`, instead of adding new public
  names.

Lots of changes are shared with PR JuliaLang#56425, if one of them gets merged
the other will be greatly simplified.
@jakobnissen
Copy link
Contributor

Do we even want to be able to sort tuples? It makes me a little uncomfortable that it's wildly type unstable to sort e.g. Tuple{Float64, Int, Int}. While this may be obvious for seasoned Julians, I worry that it will make it too easy for unexperienced programmers to accidentally create 100s, or 1000s of new types in their codebase which makes specialization explode.

@KristofferC
Copy link
Member

Indeed, I think sorting of non-homogenous tuples has very little practical use.

@vtjnash
Copy link
Member

vtjnash commented Nov 22, 2024

Indeed, I think sorting of non-homogenous tuples has very little practical use.

I would support bringing back the PR that made it easy to sort arbitrary iterables, since in this example it would give the ability to be type stable (eltype of that Tuple is type stable), so the resulting sorted object would be both fast and reliable

@KristofferC
Copy link
Member

I would support bringing back the PR that made it easy to sort arbitrary iterables,

That feature has already been discussed, tuple sorting can clearly be made withiut the requirement that arbitrary iterators can be sorted.

@stevengj
Copy link
Member

See also #31818, which points to the static sorting algorithm in TupleTools.jl

@nsajko nsajko marked this pull request as draft December 9, 2024 19:12
@nsajko nsajko closed this Dec 24, 2024
@nsajko nsajko deleted the stable_tuple_sort branch December 24, 2024 05:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Indicates new feature / enhancement requests sorting Put things in order
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Non allocating sort for tuple
8 participants