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

Speed up AsList, AsSet and ElementsStabChain for permutation groups, by not sorting the list returned by ElementsStabChain (in accordance with its documentation which never promised this) #5216

Merged
merged 1 commit into from
Dec 5, 2022

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Nov 23, 2022

The documentation for ElementsStabChain never claimed that the result it returns is sorted. So we change it to not do that, by using Append instead of UniteSet. Then we use that to provide a faster version of AsList. As a side effect, this actually even speeds up AsSet, as sorting only at the end is cheaper then sorting repeatedly during creation of the list.

Some timings:

Before:

gap> AsList(SymmetricGroup(10));; time;
1507

gap> AsSet(SymmetricGroup(10));; time;
1492

After:

gap> AsList(SymmetricGroup(10)); time;
611

gap> AsSet(SymmetricGroup(10)); time;
1120

As another side effect, the output of AsList(G) and
List(Iterator(G)) now seem to coincide if G is a permutation group.

Of course in practice it's usually a bad idea to enumerate all group elements to start with... but for some applications this is still needed.

@fingolfin fingolfin added topic: performance bugs or enhancements related to performance (improvements or regressions) topic: library release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes labels Nov 23, 2022
@fingolfin fingolfin changed the title Speed up ElementsStabChain and AsList for permutation groups Speed up AsList, AsSet and ElementsStabChain for permutation groups Nov 23, 2022
Copy link
Contributor

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

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

Changing the behaviour of a function w.r.t. sortedness of its result usually has the effect that some examples will have different outputs, thus tests have to be adjusted.
(I expect this effect in some package manuals or in files that document GAP computations.)

Nevertheless, the proposed change is a nice performance improvement.
Perhaps the fact that ElementsStabChain does no longer return a sorted result should be mentioned in the release notes.

@ChrisJefferson
Copy link
Contributor

I approve this, but I agree this should be in the release notes

@fingolfin
Copy link
Member Author

Another option would be to rename the modified ElementsStabChain to say ListStabChain, and then add a new ElementsStabChain which calls ListStabChain , sorts the result, and orders it. Though I think all calls in the library to ElementsStabChain would be removed in the end... Presumably I then should document ListStabChain...

Since no package calls ElementsStabChain, it should indeed also be sufficient to simply document the behavior change of ElementsStabChain and perhaps stress in its docstring that the result may not be sorted... (right now sorting is just not mentioned; but since Elements is a synonym for AsSet, people may infer based on that name that the result should be sorted...)

@ThomasBreuer
Copy link
Contributor

ElementsStabChain is used only in those places in lib that get changed in this pull request. I think it is not a problem to change its behaviour, to mention this in the release notes, and perhaps to state in the documentation that the result is not necessarily sorted.

(The function looks like a utility that had been introduced only in order to use it in methods for AsSSortedList and AsSSortedListNonstored. If the idea would have been to provide functionality for stabilizer chains then the version that returns a list that is ordered w.r.t. the given chain would have been more natural. Thus is had been a mistake to document ElementsStabChain.)

The documentation for `ElementsStabChain` never claimed that the result
it returns is sorted. So we change it to not do that, by using `Append`
instead of `UniteSet`. Then we use that to provide a faster version of
`AsList`. As a side effect, this actually even speeds up `AsSet`, as
sorting only at the end is cheaper then sorting repeatedly during
creation of the list.

Some timings:

Before:

    gap> AsList(SymmetricGroup(10));; time;
    1507

    gap> AsSet(SymmetricGroup(10));; time;
    1492

After:

    gap> AsList(SymmetricGroup(10)); time;
    611

    gap> AsSet(SymmetricGroup(10)); time;
    1120

As another side effect, the output of `AsList(G)` and
`List(Iterator(G))` now seem to coincide if `G` is a permutation group.
@fingolfin fingolfin changed the title Speed up AsList, AsSet and ElementsStabChain for permutation groups Speed up AsList, AsSet and ElementsStabChain for permutation groups, by not sorting the list returned by ElementsStabChain (in accordance with its documentation which never promised this) Dec 5, 2022
@fingolfin
Copy link
Member Author

@ThomasBreuer I've adjusted the docstring to mention that the result may be unsorted, and adjusted the title of this PR (which will be used for the release notes). Better now?

@ThomasBreuer
Copy link
Contributor

@fingolfin Yes, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: library topic: performance bugs or enhancements related to performance (improvements or regressions)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants