Skip to content

Make Sortex stable #2654

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.

Sign up for GitHub

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

Merged
stevelinton merged 4 commits into from
Jul 23, 2018
Merged

Make Sortex stable #2654

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3977263
Make Sortex use a stable sort
ChrisJefferson Jul 18, 2018
20debaa
Make SortingPerm use SortEx
ChrisJefferson Jul 18, 2018
94af0f9
Add tests for Sortex and SortingPerm
ChrisJefferson Jul 18, 2018
87a0d07
Improve sort documentation in lists.xml
ChrisJefferson Jul 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/list.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1661,7 +1661,7 @@ DeclareOperation( "StableSortBy", [IsList and IsMutable, IsFunction ] );
## <Oper Name="Sortex" Arg='list[, func]'/>
##
## <Description>
## sorts the list <A>list</A> and returns a permutation
## stable sorts the list <A>list</A> and returns a permutation
## that can be applied to <A>list</A> to obtain the sorted list.
## The one argument form sorts via the operator <C>&lt;</C>,
## the two argument form sorts w.r.t. the function <A>func</A>.
Expand Down
35 changes: 5 additions & 30 deletions lib/list.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2373,7 +2373,7 @@ InstallMethod( Sortex,

n := Length(list);
index := [1..n];
SortParallel(list, index);
StableSortParallel(list, index);
return PermList(index)^-1;

end );
Expand All @@ -2391,7 +2391,7 @@ InstallMethod( Sortex,

n := Length(list);
index := [1..n];
SortParallel(list, index, comp);
StableSortParallel(list, index, comp);
return PermList(index)^-1;

end );
Expand Down Expand Up @@ -2434,37 +2434,12 @@ end );
InstallMethod( SortingPerm,
[ IsDenseList ],
function( list )
local both, perm, i, l;
local copy;

# {\GAP} supports permutations only up to `MAX_SIZE_LIST_INTERNAL'.
if not IsSmallList( list ) then
Error( "<list> must have length at most ", MAX_SIZE_LIST_INTERNAL );
fi;

# make a new list that contains the elements of <list> and their indices
both := [];
l:= Length( list );
for i in [ 1 .. l ] do
both[i] := [ list[i], i ];
od;

# Sort the new list according to the first item (stable).
# This needs more memory than a call of 'Sort' but is much faster.
# (The change was proposed by Frank Lübeck.)
both := Set( both );

# Remember the permutation.
perm := [];
perm{ [ 1 .. l ] }:= both{ [ 1 .. l ] }[2];

# return the permutation mapping <list> onto the sorted list
return PermList( perm )^(-1);
copy := ShallowCopy(list);
return Sortex(copy);
end );

InstallMethod( SortingPerm,
"for a dense and sorted list",
[ IsDenseList and IsSortedList ], SUM_FLAGS,
list -> () );
Copy link
Member

@fingolfin fingolfin Jul 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChrisJefferson any particular reason this method was removed? I see no mention of this change in the PR description nor the commit message.

Copy link
Contributor Author

@ChrisJefferson ChrisJefferson Jul 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was part of making SortingPerm be exactly the same as SortEx.

Copy link
Member

@fingolfin fingolfin Jul 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

@ChrisJefferson ChrisJefferson Jul 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@ThomasBreuer ThomasBreuer Jul 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



#############################################################################
Expand Down