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

Collection of miscellaneous small changes ("cleanup") #4341

Merged
merged 6 commits into from
Mar 27, 2021

Conversation

wilfwilson
Copy link
Member

@wilfwilson wilfwilson commented Mar 25, 2021

This is a collection of several 'random' cleanup-style commits that I've made over the last year, but that have never made their way into another PR. I decided to just collect them into one PR here.

This is split into 7 commits with descriptive commit messages to help with reviewing.

@wilfwilson wilfwilson added release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes priority: low labels Mar 25, 2021
@wilfwilson wilfwilson force-pushed the ww/cleanup branch 2 times, most recently from 7618312 to 85d3237 Compare March 25, 2021 12:26
#
gap> SetUserPreference("PartialPermDisplayLimit", 1);
gap> PartialPerm([1], [2]);
<partial perm on 1 pts with degree 1, codegree 2>
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: if (when) this PR is merged, #4050 will need to be rebased and updated to change this line to

Suggested change
<partial perm on 1 pts with degree 1, codegree 2>
<partial perm on 1 pt with degree 1, codegree 2>

fi;
end );
G -> IsTrivial(G) or
ForAll(GeneratorsOfGroup(G),i->SignPerm(i)=1) and PermgpContainsAn(G)=true);
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps also like this?

Suggested change
ForAll(GeneratorsOfGroup(G),i->SignPerm(i)=1) and PermgpContainsAn(G)=true);
ForAll(GeneratorsOfGroup(G),i->SignPerm(i)=1) and PermgpContainsAn(G));

Copy link
Member Author

Choose a reason for hiding this comment

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

PermgpContainsAn can return either true or false or the return value of PermNatAnTestDetect which can be a set of points, so I can't make that change (as much as I'd like to).

lib/semitran.gd Outdated
@@ -17,8 +17,6 @@ DeclareSynonym("IsTransformationMonoid", IsMonoid and
IsTransformationCollection);

DeclareProperty("IsFullTransformationSemigroup", IsSemigroup);
InstallTrueMethod(IsSemigroup, IsFullTransformationSemigroup);
Copy link
Member

Choose a reason for hiding this comment

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

Why is this unnecessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps I misunderstand how things work.

Since IsFullTransformationSemigroup is only declared for IsSemigroup, that means anything that could know it satisfies IsFullTransformationSemigroup already knows that it is a semigroup.

Is perhaps the idea that: this InstallTrueMethod is useful because perhaps someone in a package could (for example) do DeclareProperty("IsFullTransformationSemigroup", IsMagma); and then install methods for IsMagma (which would have to somehow test the magma for associativity, but perhaps not directly, and this could therefore mean that IsFullTransformationSemigroup gets set but not IsSemigroup)?

But actually it seems that any magma of transformations already knows that it is associative, and therefore a semigroup:

gap> S := Magma(GeneratorsOfMagma(FullTransformationSemigroup(3)));;
gap> HasIsSemigroup(S);
true

I can't think of any use for this true method, but perhaps I am not imaginative enough 🙂

I notice that there are similar true methods for natural symmetric and alternating groups (e.g. in lib/gpprmsya.gd):

InstallTrueMethod( IsPermGroup, IsNaturalSymmetricGroup );

But things seem to work differently for groups:

gap> G := Semigroup(GeneratorsOfMagma(SymmetricGroup(4)));;
gap> IsGroup(G);
false
gap> SetIsNaturalSymmetricGroup(G, true);
gap> IsGroup(G);
true

Copy link
Member

Choose a reason for hiding this comment

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

There is no automatic implication and there cannot be, as indeed GAP cannot predict the future and so can't know whether that property might in the future also be applicable to, say, algebras or strings or graphs or... Sure, as a human you know this is unlikely because of the name, but GAP does not known this (and anyway.

Somewhat related: issue #2336

Copy link
Member Author

@wilfwilson wilfwilson Mar 25, 2021

Choose a reason for hiding this comment

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

(and anyway.

Were you going to add something?

Anyway, I'll be happy to remove I have happily removed this commit. I'll read #2336 too.

@fingolfin fingolfin merged commit 29cab2d into gap-system:master Mar 27, 2021
@wilfwilson wilfwilson deleted the ww/cleanup branch March 28, 2021 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: low release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants