Improve documentation of sort-related functions #48387
Conversation
knuesel
force-pushed
the
doc-sort-order
branch
from
7d3ff90 to
a91d0ed
Compare
LilithHafner
added
docs
There was a problem hiding this comment.
Thanks so much! I appreciate your work on this and think it is orthogonal/compatable with #48363, so we can certainly merge both. I've left some comments and will be back with more soon.
There was a problem hiding this comment.
Most of this PR is documentation improvements and fixes that could be but do not need to be backported to 1.9.
There are also some fixups for docstrings introduced in 1.9, which should be backported, and a strengthened guarantee of the correctness of searchsorted* family which should probably not be backported.
I imagine it would be best to split this into two PRs, one for changes that should be backported to 1.9 (low risk, unambiguous fixes, especially for docstrings introduced in 1.9) and changes that should first appear in 1.10 (anything that could plausibly introduce new problems than it solves)
Pinging @KristofferC who is managing most of the backports to 1.9 to see if you have a different recommendation.
knuesel
force-pushed
the
doc-sort-order
branch
from
56bc252 to
9481be3
Compare
|
Thanks @LilithHafner for the review. I have removed the strengthened guarantee o correctness of In the latest version I have reworded a bunch of dosctring so that all the formalities are centralized in (And I rebased on latest master by mistake so the "Compare" button above shows unrelated broadcast changes and the "Compare" below is worse, sorry...) |
knuesel
force-pushed
the
doc-sort-order
branch
2 times, most recently
from
e082e14 to
dc77bca
Compare
I think this is a typo EDIT: fixed |
In particular: * document the `order` keyword in `sort!` * list explicitly the required properties of `lt` in `sort!` * clarify the sequence of "by" transformations if both `by` and `order` are given * show default values in the signatures for `searchsorted` and related functions * add `isunordered` to the manual (it's already exported)
In particular: * document the `order` keyword in `sort!` * list explicitly the required properties of `lt` in `sort!` * clarify the sequence of "by" transformations if both `by` and `order` are given * show default values in the signatures for `searchsorted` and related functions * note that `by` is also applied to searched value in `searchsorted` and related * add `isunordered` to the manual (it's already exported)
|
@LilithHafner do you want to have another look at this now that 1.9 is released? I've rebased on master and made the changes mentioned in #48449. I've also added a note to the |
|
Gentle bump |
There was a problem hiding this comment.
We lost
rev=truepreserves forward stability: Elements that compare equal are not reversed."
and a definition of what stability is in the new sort! docstring
Aside from that and the inline comments I left I think this is ready to go (and good to backport to 1.10)
Sorry about the incredible delay.
LilithHafner
added
the
backport 1.10
Co-authored-by: Lilith Orion Hafner <lilithhafner@gmail.com>
Co-authored-by: Lilith Orion Hafner <lilithhafner@gmail.com>
Co-authored-by: Lilith Orion Hafner <lilithhafner@gmail.com>
Co-authored-by: Lilith Orion Hafner <lilithhafner@gmail.com>
Co-authored-by: Lilith Orion Hafner <lilithhafner@gmail.com>
|
Thanks for the comments, I've added back the lost part about |
|
Lovely, thank you so much! (The "definition of stability" I was referring to was "Elements that compare equal are not reversed.") |
* document the `order` keyword in `sort!` * list explicitly the required properties of `lt` in `sort!` * clarify the sequence of "by" transformations if both `by` and `order` are given * show default values in the signatures for `searchsorted` and related functions * note that `by` is also applied to searched value in `searchsorted` and related * add `isunordered` to the manual (it's already exported) --------- Co-authored-by: Lilith Orion Hafner <lilithhafner@gmail.com> (cherry picked from commit a660798)
Backported PRs: - [x] #48625 <!-- add replace(io, str, patterns...) --> - [x] #48387 <!-- Improve documentation of sort-related functions --> - [x] #48363 <!-- Revise sort.md and docstrings in sort.jl --> - [x] #48977 <!-- Update SparseArrays.jl stdlib for SuiteSparse 7 --> - [x] #50719 <!-- fix `CyclePadding(::DataType)` --> - [x] #50694 <!-- inference: permit recursive type traits --> - [x] #50860 <!-- Add `Base.get_extension` to docs/API --> - [x] #50594 <!-- Disallow non-index Integer types in isassigned --> - [x] #50802 <!-- Makes IntrusiveLinkedListSynchronized mutable to avoid allocation on poptask --> - [x] #50858 <!-- Add a `threadpool` parameter to `Channel` constructor --> - [x] #50874 <!-- Restrict COFF to a single thread when symbol count is high --> - [x] #50822 <!-- Add default method for setmodifiers! --> - [x] #50730 <!-- Fix integer overflow in `isapprox` --> - [x] #50850 <!-- Remove weird Rational dispatch and add pi functions to list --> - [x] #50809 <!-- Limit type-printing in MethodError --> - [x] #50915 <!-- Add note the `Task` about sticky bit --> - [x] #50929 <!-- when widening tuple types in tmerge, only widen the complex parts --> - [x] #50928 <!-- Bump JuliaSyntax to 0.4.6 --> - [x] #50959 <!-- Update libssh2 patches --> - [x] #50823 <!-- Make ranges more robust with unsigned indexes. --> - [x] #48542 <!-- Add docs on task-specific buffering using multithreading --> - [x] #50912 <!-- Separate foreign threads into a :foreign threadpool --> - [x] #51010 <!-- Add ORIGIN to SuiteSparse rpath on Linux/FreeBSD --> - [x] #50753 <!-- cat: remove unused promote_eltype methods that confuse inference --> - [x] #51027 <!-- Implement realloc accounting correctly --> - [x] #51019 <!-- fix a case of potentially use of undefined variable when handling error in distributed message processing --> - [x] #51039 <!-- Revert "Optimize findall(f, ::AbstractArray{Bool}) (#42202)" --> - [x] #51036 <!-- add missing invoke edge for nospecialize targets --> - [x] #51042 <!-- inference: fix return_type_tfunc modeling of concrete functions --> - [x] #51114 <!-- Workaround upstream FreeBSD issue #272992 --> - [x] #50892 <!-- Add `JL_DLLIMPORT` to `small_typeof` declaration --> - [x] #51154 <!-- broadcast: use recursion rather than ntuple to map over a tuple --> - [x] #51153 <!-- fix debug typo in "add missing invoke edge for nospecialize targets (#51036)" --> - [x] #51222 <!-- Check again if the tty is open inside the IO lock --> - [x] #51236 <!-- Add lock around uv_unref during init --> - [x] #51243 <!-- GMP: Gracefully handle more overflows. --> - [x] #51254 <!-- Ryu: make sure adding zeros does not overwrite trailing dot --> - [x] #51175 <!-- shorten stale_age for cachefile lock --> - [x] #51300 <!-- fix method definition error for bad vararg --> - [x] #51307 <!-- fix force-throw ctrl-C on Windows --> - [x] #51303 <!-- ensure revising structs is safe --> - [x] #51393 - [x] #51403 Need manual backport: - [x] #51009 <!-- fix #50562, regression in `in` of tuple of Symbols --> - [x] #51053 <!-- Bump Statistics stdlib --> - [x] #51013 <!-- fix #50709, type bound error due to free typevar in sparam env --> - [x] #51305 <!-- reduce test time for rounding and floatfuncs --> Contains multiple commits, manual intervention needed: - [ ] #50663 <!-- Fix Expr(:loopinfo) codegen --> - [ ] #51035 <!-- refactor GC scanning code to reflect jl_binding_t are now first class --> - [ ] #51092 <!-- inference: fix bad effects for recursion --> - [x] #51247 <!-- Check if malloc has succeeded before incrementing gc stats --> - [x] #51294 <!-- use LibGit2_jll for LibGit2 library --> Non-merged PRs with backport label: - [ ] #51132 <!-- Handle `AbstractQ` in concatenation --> - [x] #51029 <!-- testdefs: make sure that if a test set changes the active project, they change it back when they're done --> - [ ] #50919 <!-- Code loading: do the "skipping mtime check for stdlib" check regardless of the value of `ispath(f)` --> - [ ] #50824 <!-- Add some aliasing warnings to docstrings for mutating functions --> - [x] #50385 <!-- Precompile pidlocks: add to NEWS and docs --> - [ ] #49805 <!-- Limit TimeType subtraction to AbstractDateTime -->
KristofferC
removed
the
backport 1.10
…iaLang#48387 "Total order" -> "strict weak order".
"Total order" -> "strict weak order".
* document the `order` keyword in `sort!` * list explicitly the required properties of `lt` in `sort!` * clarify the sequence of "by" transformations if both `by` and `order` are given * show default values in the signatures for `searchsorted` and related functions * note that `by` is also applied to searched value in `searchsorted` and related * add `isunordered` to the manual (it's already exported) --------- Co-authored-by: Lilith Orion Hafner <lilithhafner@gmail.com> (cherry picked from commit a660798)
In particular:
orderkeyword insort!,ltinsort!,byandorderare given,searchsortedand related functions,byis also applied to searched value insearchsortedand related,isunorderedto the manual (it's already exported),sort!the current behavior of callingbyfunctions for each comparison rather than once per element.I think this fixes #27661, fixes #44102 and fixes #34947.