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

Change GroupWithGenerators to accept collections again #3095

Merged

Conversation

fingolfin
Copy link
Member

In GAP <= 4.9, this worked:

gap> GroupByGenerators( Group( (1,2) ) );
Group([ (), (1,2) ])

In GAP 4.10, this was broken and instead lead to a "method not found" error.
While strictly speaking this was never documented behavior, we restore it
to avoid regressions in code that relied on this undocumented behavior.

Resolves #2703

This is a less intrusive (and thus hopefully less controversial) alternative to PR #3091. Closes #3091

@fingolfin fingolfin added kind: bug Issues describing general bugs, and PRs fixing them kind: bug: unexpected error Issues describing bugs in which computation unexpectedly encounters an error, and PRs fixing them regression A bug that only occurs in the branch, not in a release backport-to-4.10 labels Dec 6, 2018
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.

These changes fix the behavior, fine.
My only suggestion is to omit the AsList calls
inside the ObjectifyWithAttributes calls,
since AsList has been applied already.

Copy link
Member

@olexandr-konovalov olexandr-konovalov left a comment

Choose a reason for hiding this comment

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

Frst off, this modifies both GroupByGenerators and GroupWithGenerators but the commit message mentions only the former one.

Furthermore, I am not convinced that calling AsList is a good idea. Imagine if someone asks, for example:

GroupByGenerators(SymmetricGroup(20))

Are we seriously expecting AsList(SymmetricGroup(20)) to complete? ;-) By the way, this applies to both PRs, as the same call to AsList is present in #3091.

The other idea suggested elsewhere was to check if the argument is a domain which knows a list of its generators, and in this case call GroupByGenerators on that list. At least that would allow to avoid expanding domain to a list of its elements. But again, to be done consistently, this should be done for all *ByGenerators methods... and perhaps also for *WithGenerators? Hmm... that would also seem controversial - but if done, then the use like that must be documented. However, I'd prefer to not do do it at all. I do think that a proper solution would be just to make a new CTblLib release. That's the only known manifestation of this problem I have discovered so far.

@ChrisJefferson
Copy link
Contributor

I don't like the idea that is something doesn't break any library code, that's fine. There are many thousands of GAP users out in the world, all with their own code. If (as in this case) it is reasonable for us not to break existing code, which has worked for many years, then I think we should try to do so -- particularly when the error message is hard to read.

There are many places in GAP where you can put objects which can be enumerated instead of an explicit list, and in any of those places SymmetricGroup(20) would be a bad idea.

@fingolfin fingolfin force-pushed the mh/fix-GroupByGenerators-for-domain branch from 9a825e4 to ea06288 Compare December 6, 2018 15:04
@codecov
Copy link

codecov bot commented Dec 6, 2018

Codecov Report

Merging #3095 into master will decrease coverage by 0.22%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3095      +/-   ##
==========================================
- Coverage   83.56%   83.33%   -0.23%     
==========================================
  Files         686      686              
  Lines      336733   336735       +2     
==========================================
- Hits       281380   280610     -770     
- Misses      55353    56125     +772
Impacted Files Coverage Δ
lib/grp.gi 88.96% <100%> (ø) ⬆️
src/compiler.c 66.34% <0%> (-22.49%) ⬇️
src/vars.c 81.28% <0%> (-12.19%) ⬇️
src/stats.c 85.71% <0%> (-9.6%) ⬇️
src/error.c 75.47% <0%> (-9.58%) ⬇️
src/exprs.c 92.9% <0%> (-4.44%) ⬇️
src/intrprtr.c 94.8% <0%> (-4.09%) ⬇️
src/funcs.c 95.39% <0%> (-2.46%) ⬇️
src/opers.c 92.96% <0%> (-2.21%) ⬇️
src/sysmem.c 55.88% <0%> (-1.77%) ⬇️
... and 9 more

In GAP <= 4.9, this worked:

    gap> GroupWithGenerators( Group( (1,2) ) );
    Group([ (), (1,2) ])

In GAP 4.10, this was broken and instead lead to a "method not found" error.
While strictly speaking this was never documented behavior, we restore it
to avoid regressions in code that relied on this undocumented behavior.

Resolves gap-system#2703
@fingolfin fingolfin force-pushed the mh/fix-GroupByGenerators-for-domain branch from ea06288 to d42fa06 Compare December 6, 2018 15:05
@fingolfin fingolfin changed the title Change GroupByGenerators to accept collections again Change GroupWithGenerators to accept collections again Dec 6, 2018
@olexandr-konovalov
Copy link
Member

@ChrisJefferson there is still an alternative:

The other idea suggested elsewhere was to check if the argument is a domain which knows a list of its generators, and in this case call GroupByGenerators on that list.

I would be happy with one of the following:

  • document the alternative behaviour as above, or

  • make a better error message when it's called with a domain, or

  • accept this PR with the following modification: if it called on a domain, then it does everything like it does now (i.e. calls AsList) but prints a warning:

    #I Calling GroupByGenerators on a domain is very inefficient, use the list of generators instead.

Anyone in favour?

@fingolfin
Copy link
Member Author

@ThomasBreuer ah, thanks for pointing out the extra AsList calls; they are now removed

@alex-konovalov this only modifies two methods for GroupWithGenerators, hence I only mention that in the commit message (well, now at least, I had GroupByGenerators in there and in the title of this PR before by accident). Of course this then also affects any code calling or delegating to GroupWithGenerators, such as GroupByGenerators.

Of course GroupByGenerators(SymmetricGroup(20)) is not sensible, but so what? This just restores behaviour that was present in GAP 4.9, while not affecting anything else. In particular, any "sane" code which only passes in regular list of generators sees no change in behavior.

Put another way: any code that currently is "correct" and "adheres to the documentation" will work unchanged. All this PR does is to restore a behavior of GAP that was present for decades (I just checked, GAP 4.4 supports it). I see zero harm in it. Note that I am not suggesting to document this behavior, either, but I don't see how we benefit from gratuitously breaking code that was working before?

@olexandr-konovalov
Copy link
Member

@fingolfin This will not fix #2703 because that one goes into the break loop because of GroupByGenerators.

@fingolfin
Copy link
Member Author

fingolfin commented Dec 6, 2018

@alex-konovalov while I wouldn't mind that warning, IMHO it should be added in a separate PR. This PR is purely about restoring behaviour that was present in GAP for the past 20+ years. To this end, I strove for a minimal set of changes. Any further changes and optimizations, documentation etc. are IMHO beyond the scope of this.

And as I said before: GroupByGenerators delegates to GroupWithGenerators. I even added a .tst file which verifies this.

Copy link
Member

@olexandr-konovalov olexandr-konovalov left a comment

Choose a reason for hiding this comment

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

Oh yes, thanks for pointing out, of course, one delegates to another.

The new commit message looks correct, so I will approve this now, and agree that we keep this undocumented. I will make a PR with a warning if this PR will pass the testing round.

@fingolfin fingolfin merged commit 72d2c36 into gap-system:master Dec 6, 2018
@fingolfin fingolfin deleted the mh/fix-GroupByGenerators-for-domain branch December 6, 2018 19:53
olexandr-konovalov pushed a commit to olexandr-konovalov/gap that referenced this pull request Dec 7, 2018
In GAP <= 4.9, the following undocumented use of GroupWithGenerators
was possible:

    gap> GroupWithGenerators( Group( (1,2) ) );
    Group([ (), (1,2) ])

This does not work in GAP 4.10.0, and gap-system#3095 restored this never documented
behavior to avoid regressions in code that used to work previously.

This commit adds a warning when such use of `GroupWithGenerators` is detected.
olexandr-konovalov pushed a commit to olexandr-konovalov/gap that referenced this pull request Dec 7, 2018
In GAP <= 4.9, the following undocumented use of GroupWithGenerators
was possible:

    gap> GroupWithGenerators( Group( (1,2) ) );
    Group([ (), (1,2) ])

This does not work in GAP 4.10.0, and gap-system#3095 restored this never documented
behavior to avoid regressions in code that used to work previously.

This commit adds a warning when such use of `GroupWithGenerators` is detected.
olexandr-konovalov pushed a commit to olexandr-konovalov/gap that referenced this pull request Dec 7, 2018
In GAP <= 4.9, the following undocumented use of GroupWithGenerators
was possible:

    gap> GroupWithGenerators( Group( (1,2) ) );
    Group([ (), (1,2) ])

This does not work in GAP 4.10.0, and gap-system#3095 restored this never documented
behavior to avoid regressions in code that used to work previously.

This commit adds a warning when such use of `GroupWithGenerators` is detected.
olexandr-konovalov pushed a commit to olexandr-konovalov/gap that referenced this pull request Dec 7, 2018
In GAP <= 4.9, the following undocumented use of GroupWithGenerators
was possible:

    gap> GroupWithGenerators( Group( (1,2) ) );
    Group([ (), (1,2) ])

This does not work in GAP 4.10.0, and gap-system#3095 restored this never documented
behavior to avoid regressions in code that used to work previously.

This commit adds a warning when such use of `GroupWithGenerators` is detected.
olexandr-konovalov pushed a commit to olexandr-konovalov/gap that referenced this pull request Dec 8, 2018
In GAP <= 4.9, the following undocumented use of GroupWithGenerators
was possible:

    gap> GroupWithGenerators( Group( (1,2) ) );
    Group([ (), (1,2) ])

This does not work in GAP 4.10.0, and gap-system#3095 restored this never documented
behavior to avoid regressions in code that used to work previously.

This commit adds a warning when such use of `GroupWithGenerators` is detected.
olexandr-konovalov pushed a commit to olexandr-konovalov/gap that referenced this pull request Dec 8, 2018
In GAP <= 4.9, the following undocumented use of GroupWithGenerators
was possible:

    gap> GroupWithGenerators( Group( (1,2) ) );
    Group([ (), (1,2) ])

This does not work in GAP 4.10.0, and gap-system#3095 restored this never documented
behavior to avoid regressions in code that used to work previously.

This commit adds a warning when such use of `GroupWithGenerators` is detected.
@fingolfin
Copy link
Member Author

Backported via ac82a50

@olexandr-konovalov olexandr-konovalov added this to the GAP 4.10.1 milestone Dec 16, 2018
@olexandr-konovalov olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Feb 23, 2019
olexandr-konovalov pushed a commit that referenced this pull request May 28, 2019
In GAP <= 4.9, the following undocumented use of GroupWithGenerators
was possible:

    gap> GroupWithGenerators( Group( (1,2) ) );
    Group([ (), (1,2) ])

This does not work in GAP 4.10.0, and #3095 restored this never documented
behavior to avoid regressions in code that used to work previously.

This commit adds a warning when such use of `GroupWithGenerators` is detected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-4.10-DONE kind: bug: unexpected error Issues describing bugs in which computation unexpectedly encounters an error, and PRs fixing them kind: bug Issues describing general bugs, and PRs fixing them regression A bug that only occurs in the branch, not in a release release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants