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

Fix ViewString method for inverse monoids without generators as an inverse semigroup/monoid #880

Merged
merged 1 commit into from
Aug 10, 2016

Conversation

james-d-mitchell
Copy link
Contributor

Description of changes (for the release notes)

Previously for inverse monoids with GeneratorsOfSemigroup but not with GeneratorsOfInverseSemigroup the wrong method for ViewString was selected, i.e. the method for ViewString of an inverse monoid (with no generators) had higher priority than the intended method.

Before this commit:

gap> S := Monoid(Transformation([1, 2, 3, 4, 5, 6, 7, 7, 7]),
>                Transformation([4, 6, 3, 6, 6, 6, 7, 7, 7]),
>                Transformation([4, 5, 6, 1, 6, 6, 7, 7, 7]),
>                Transformation([6, 6, 3, 1, 6, 6, 7, 7, 7]),
>                Transformation([4, 6, 6, 1, 2, 6, 7, 7, 7]));;
gap> IsInverseSemigroup(S);
true
gap> S;
<inverse monoid>

after:

gap> S := Monoid(Transformation([1, 2, 3, 4, 5, 6, 7, 7, 7]), 
>                Transformation([4, 6, 3, 6, 6, 6, 7, 7, 7]),
>                Transformation([4, 5, 6, 1, 6, 6, 7, 7, 7]), 
>                Transformation([6, 6, 3, 1, 6, 6, 7, 7, 7]), 
>                Transformation([4, 6, 6, 1, 2, 6, 7, 7, 7]));;
gap> IsInverseSemigroup(S);
true
gap> S;
<inverse transformation monoid of size 18, degree 9 with 5 generators>

Specifically for inverse monoids with GeneratorsOfSemigroup but not with
GeneratorsOfInverseSemigroup.  Before this commit, the method for
ViewString of an inverse monoid (with no generators) had higher
priority than the intended method.
@codecov-io
Copy link

Current coverage is 49.06% (diff: 100%)

Merging #880 into master will increase coverage by <.01%

@@             master       #880   diff @@
==========================================
  Files           422        422          
  Lines        228711     228711          
  Methods        3447       3447          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits         112221     112227     +6   
+ Misses       116490     116484     -6   
  Partials          0          0          

Powered by Codecov. Last update 042402e...8db6c8b

@olexandr-konovalov
Copy link
Member

@james-d-mitchell thanks, looks good to me.

@markuspf markuspf merged commit 55d9ff2 into gap-system:master Aug 10, 2016
@olexandr-konovalov
Copy link
Member

@james-d-mitchell now we have a diff in the test with all packages loaded:

########> Diff in /mnt/disk2/hudson-slave/workspace/GAP-dev/GAPCOPTS/64build/G\
APGMP/gmp/GAPTARGET/standard/label/64bit/GAP-git-snapshot/tst/testinstall/semi\
grp.tst:456
# Input is:
S;
# Expected output:
<inverse transformation monoid of size 18, degree 9 with 5 generators>
# But found:
<inverse transformation monoid of degree 9 with 5 generators>
########

@james-d-mitchell
Copy link
Contributor Author

This is (probably) caused by the different methods for IsInverseSemigroup in the Semigroups package and in GAP. This can be easily fixed by adding the line gap> Size(S);; to the test file before the S;.

Should I amend the PR accordingly?

@olexandr-konovalov
Copy link
Member

@james-d-mitchell thanks - since the PR has been merged already, I can adjust the test myself.

@james-d-mitchell
Copy link
Contributor Author

@alex-konovalov ok great!

olexandr-konovalov pushed a commit that referenced this pull request Aug 12, 2016
@olexandr-konovalov
Copy link
Member

Updated test example in 80a0966

james-d-mitchell pushed a commit to james-d-mitchell/gap that referenced this pull request Aug 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants