Make Sortex stable #2654
Make Sortex stable #2654
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2654 +/- ##
==========================================
- Coverage 75.19% 75.19% -0.01%
==========================================
Files 478 478
Lines 242226 242214 -12
==========================================
- Hits 182134 182124 -10
+ Misses 60092 60090 -2
|
There was a problem hiding this comment.
This looks good.
(I would call ShallowCopy directly instead of the one argument version of List, but this is perhaps just a matter of taste.)
Concerning the documentation, I am wondering whether the paragraph The default methods for all of these sorting operations ... from doc/ref/lists.xml is (a) still correct (since it is far away from the actual code) and (b) in a good place (I think such a statement would better appear in the beginning of the section Sorting Lists).
|
I agree with using |
| merge sort. Finally, the functions which begin <C>Shell</C> use the shell | ||
| sort which was GAP's default search algorithm before 4.9. | ||
|
|
||
| <Ref Oper="Sortex"/> and <Ref Attr="SortingPerm"/> are also stable. |
There was a problem hiding this comment.
I would move the paragraph to the beginning of the manual section on Sorting Lists.
In particular for someone reading the manual (and not the source),
it is not clear that this paragraph belongs to all subsections and not just to the last one.
There was a problem hiding this comment.
Now done. Also mentioned better in the documentation for sortex that it is stable.
There was a problem hiding this comment.
Great.
Thanks for this improvement.
| merge sort. Finally, the functions which begin <C>Shell</C> use the shell | ||
| sort which was GAP's default search algorithm before 4.9. | ||
|
|
||
| <Ref Oper="Sortex"/> and <Ref Attr="SortingPerm"/> are also stable. |
There was a problem hiding this comment.
Great.
Thanks for this improvement.
| InstallMethod( SortingPerm, | ||
| "for a dense and sorted list", | ||
| [ IsDenseList and IsSortedList ], SUM_FLAGS, | ||
| list -> () ); |
There was a problem hiding this comment.
@ChrisJefferson any particular reason this method was removed? I see no mention of this change in the PR description nor the commit message.
There was a problem hiding this comment.
It was part of making SortingPerm be exactly the same as SortEx.
There was a problem hiding this comment.
I don't see why that requires removing this optimization, though? Now if the input is sorted, we make a copy, sort that, and compute the identity permutation from that...
There was a problem hiding this comment.
It could probably have been left in, I wouldn't have a problem with it being re-added but I wasn't sure it was worth it, for a fairly unusual case.
There was a problem hiding this comment.
I see this method as an illustration of the idea to avoid working by using known information.
If the presence of such methods for fairly unusual cases does not slow down
the computations in the usual cases
then these methods are beneficial.
If this is not the case then it might be interesting to discuss the situation in detail,
perhaps in some tutorial.
fingolfin
added
the
release notes: to be added
fingolfin
added
kind: enhancement
This patch makes sortex stable. As it is now stable and efficient, make SortingPerm use sortex.