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

Special case identity permutations and improve printing of permutations #3566

Merged
merged 2 commits into from
Jul 15, 2019

Conversation

ChrisJefferson
Copy link
Contributor

@ChrisJefferson ChrisJefferson commented Jul 14, 2019

Resolves #3547

This adds special cases for identity permutations in various methods. These do get called fairly often, and are very cheap.

EDIT: This now fixes another problem found along the way. GAP has the internal concept of the 'degree' of a permutation, which can be larger than the LargestMovedPoint. For example, p=(1,2) has DEG_PERM=2, but q=(2^19,2^20)*(1,2)*(2^19,2^20) has DEG_PERM=2^20, as GAP doesn't try hard to reduce this value, but just uses a weak guess.

This value is used in printing to decide how many digits we need when printing, so the permutations p and q print differently, even though from the GAP language there is no way to tell them apart. This could break tests in some packages, who have made permutations with a large DEG_PERM

 gap> p := (1,2);
(1,2)
gap> q := (1,2)*(2^19,2^20)*(2^19,2^20);
(1,2)
gap> Print(p);
(1,2)
gap> Print(q);
(    1,    2)
gap> p=q;
true

Text for release notes

  • Operations involving identity permutations optimised.
  • Improve printing of permutations

@ChrisJefferson ChrisJefferson added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Jul 14, 2019
@codecov
Copy link

codecov bot commented Jul 14, 2019

Codecov Report

Merging #3566 into master will decrease coverage by 8.89%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3566      +/-   ##
==========================================
- Coverage   84.53%   75.64%    -8.9%     
==========================================
  Files         696      644      -52     
  Lines      345073   296213   -48860     
==========================================
- Hits       291716   224067   -67649     
- Misses      53357    72146   +18789
Impacted Files Coverage Δ
src/permutat.cc 85.38% <100%> (-11.75%) ⬇️
src/fibhash.h 0% <0%> (-100%) ⬇️
src/syntaxtree.c 5.44% <0%> (-90.72%) ⬇️
lib/meatauto.gi 5.99% <0%> (-89.32%) ⬇️
lib/nilpquot.gi 11.11% <0%> (-88.89%) ⬇️
src/modules.h 16.66% <0%> (-83.34%) ⬇️
lib/helpt2t.gi 0.23% <0%> (-83.14%) ⬇️
src/dteval.c 3.3% <0%> (-80.12%) ⬇️
src/objset.c 8.13% <0%> (-76.74%) ⬇️
lib/dt.g 7.75% <0%> (-72.2%) ⬇️
... and 419 more

@ChrisJefferson ChrisJefferson changed the title Special case identity permutations Special case identity permutations and improve printing of permutations Jul 14, 2019
@coveralls
Copy link

coveralls commented Jul 14, 2019

Coverage Status

Coverage increased (+0.001%) to 84.316% when pulling 1461dba on ChrisJefferson:perm-id into d9b9d3a on gap-system:master.

@fingolfin
Copy link
Member

Is there a strong reason to combine a potentially breaking change with an enhancement in one PR? I think we might want to merge one immediately, but for the other take some more time to first let Alex run all package tests and determine if we break some packages, and then give the package authors a chance to adapt (or even help them fix their packages)?

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

The enhancement seems obvious enough (how useful it is in practice is another question, but I'd expect little harm). I am reluctant about merging it mixed in with a potentially breaking change to printing, though (even though the "break" should "only" affect package tests).

src/permutat.cc Show resolved Hide resolved
src/permutat.cc Show resolved Hide resolved
src/permutat.cc Outdated Show resolved Hide resolved
src/permutat.cc Show resolved Hide resolved
src/permutat.cc Show resolved Hide resolved
tst/testinstall/trans.tst Outdated Show resolved Hide resolved
@ChrisJefferson
Copy link
Contributor Author

I'll have a rearrange. The two issues ended up merged because I only discovered the problem with printing when testing this.

@ChrisJefferson
Copy link
Contributor Author

So, I looked at separating this, but I think it might be best to test them together, as they can both produce visible differences in tests, so if we separate we just have to make two passes over the packages.

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Sadly tests seem to be failing?

src/permutat.cc Outdated Show resolved Hide resolved
src/permutat.cc Show resolved Hide resolved
src/permutat.cc Outdated Show resolved Hide resolved
src/permutat.cc Outdated Show resolved Hide resolved
src/permutat.cc Outdated Show resolved Hide resolved
src/permutat.cc Show resolved Hide resolved
src/permutat.cc Outdated Show resolved Hide resolved
src/permutat.cc Show resolved Hide resolved
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Great! I have one tiny further optimization suggestion, but it's also OK to merge this directly, you are probably fed enough by my niggling :-). Thank you for making this perfect

src/permutat.cc Outdated Show resolved Hide resolved
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Still great. And as to potentially breaking package tests, I guess we can argue that they were not very robust to start with ;-). After merging this, let's try to watch the Travis package tests at https://travis-ci.org/gap-infra/gap-docker-pkg-tests-master in the coming days for new failures

@fingolfin fingolfin merged commit 8c9f355 into gap-system:master Jul 15, 2019
@wucas wucas 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 20, 2019
@ChrisJefferson ChrisJefferson deleted the perm-id branch August 23, 2019 10:55
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

Special casing identity in permutation operations
5 participants