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.

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

Jump to bottom

Show a warning when GroupWithGenerators called on a domain #3099

Merged
merged 1 commit into from
May 28, 2019
Merged

Show a warning when GroupWithGenerators called on a domain #3099

merged 1 commit into from
May 28, 2019

Conversation

olexandr-konovalov
Copy link
Member

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 PR adds a warning when such use of GroupWithGenerators is detected.

fingolfin
fingolfin reviewed Dec 7, 2018
View reviewed changes
lib/grp.gi Outdated Show resolved Hide resolved
lib/grp.gi Outdated Show resolved Hide resolved
@olexandr-konovalov olexandr-konovalov force-pushed the info-performance-grp branch 2 times, most recently from 1677996 to e941054 Compare December 7, 2018 12:52
@markuspf
Copy link
Member

markuspf commented Dec 8, 2018

But... why?

@olexandr-konovalov
Copy link
Member Author

@markuspf what do you mean by "why?"? The commit message explains the reasons, and you may also see the discussion in #3095 and related PRs/issues.

InfoPerformance sounds like an optimal info level for such messages, and it's default value is 1 when GAP starts, but is set to 0 in regression tests, so this message will not show up there.

@ChrisJefferson
Copy link
Contributor

Are you sure this mesage is true for all domains? I think could make a domain where taking its generators and making a group from them would produce a different answer (although in such a case the user is doing something very strange...)

@olexandr-konovalov
Copy link
Member Author

@ChrisJefferson Do you think it should only check for IsGroup instead of IsDomain? Indeed, the way how the domain is generated by its generators may be different.

@ChrisJefferson
Copy link
Contributor

Group feels like an easy case to catch -- as previously discussed we could even then just special case groups, but then we are going in a loop :) I don't know enough about how domains are defined to know if there is a good way of catching and warning about exactly the right things.

@codecov
Copy link

codecov bot commented Dec 8, 2018 •
edited
Loading

Codecov Report

Merging #3099 into master will decrease coverage by 8.39%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master    #3099      +/-   ##
==========================================
- Coverage   83.56%   75.16%    -8.4%     
==========================================
  Files         686      680       -6     
  Lines      336759   328485    -8274     
==========================================
- Hits       281401   246900   -34501     
- Misses      55358    81585   +26227
Impacted Files Coverage Δ
lib/grp.gi 79.1% <33.33%> (-9.87%) ⬇️
lib/teachm2.g 15.38% <0%> (-84.62%) ⬇️
src/modules.h 16.66% <0%> (-83.34%) ⬇️
lib/proto.gi 1.03% <0%> (-82.48%) ⬇️
lib/ctbllatt.gi 0.81% <0%> (-79.99%) ⬇️
lib/ctblauto.gi 5.98% <0%> (-78.04%) ⬇️
lib/algliess.gi 0.99% <0%> (-74.54%) ⬇️
lib/attr.gi 27.27% <0%> (-72.73%) ⬇️
lib/ctblpope.gi 1.77% <0%> (-72.39%) ⬇️
lib/contfrac.gi 30% <0%> (-67.15%) ⬇️
... and 287 more

@olexandr-konovalov
Copy link
Member Author

Indeed,GeneratorsOfDomain for a domain which is not a group may be generators w.r.t. different operation(s), so for that case it might have sense to use GroupWithGenerators on the whole domain. I've pushed the new version which uses IsGroup instead of IsDomain.

Show a warning when GroupWithGenerators called on a group
79a636c 
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
fingolfin reviewed Dec 11, 2018
View reviewed changes
lib/grp.gi
Info( InfoPerformance, 1,
"Calling `GroupWithGenerators' on a group usually is very inefficient.");
Info( InfoPerformance, 1,
"Use the list of generators of the group instead.");
Copy link
Member

Choose a reason for hiding this comment

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

But isn't the actual problem that using a large number of generators is usually inefficient? Then, shouldn't the check instead be on the size of gens? And then, shouldn't it apply equally to lists as to domains or any other kind of collection?

Copy link
Member Author

Choose a reason for hiding this comment

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

But how then would you know when the number is large? I assume that the number of generators is usually smaller than the number of elements in the group (unless it is already given using a very inefficient list of generators).

Copy link
Member

Choose a reason for hiding this comment

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

Sure, any number we'd pick as "large" would be arbitrary. Just like it is arbitrary to allow Group(AsList(G)) as generators, but not Group(G). So my personal preference would be to do neither, i.e., exactly as it is right now...

Copy link
Member Author

Choose a reason for hiding this comment

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

Can't think of any scenario when calling Group(Group(...)) is more efficient than Group(GeneratorsOfGroup(...)) - that's what I am suggesting to catch. Not other inefficiencies, as that would be more ambiguous...

@wucas wucas added the kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements label Mar 22, 2019
@fingolfin fingolfin added the release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes label May 24, 2019
@olexandr-konovalov olexandr-konovalov merged commit d766e42 into gap-system:master May 28, 2019
@olexandr-konovalov olexandr-konovalov deleted the info-performance-grp branch May 28, 2019 09:54
@DominikBernhardt DominikBernhardt added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 22, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fingolfin fingolfin fingolfin approved these changes

Assignees
No one assigned
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Milestone
GAP 4.11.0
Development

Successfully merging this pull request may close these issues.
6 participants
@olexandr-konovalov @markuspf @ChrisJefferson @fingolfin @DominikBernhardt @wucas