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

Add Translations #720

Merged
merged 92 commits into from
Sep 30, 2022
Merged

Conversation

flsmith
Copy link
Collaborator

@flsmith flsmith commented Dec 2, 2020

This adds functionality for translations; see the description of #373.

Remaining tasks:

  • Classify the small number of remaining TODOs
  • Allow computation of the number of (bi)translations without generating them; most of the code required for this already exists
  • Complete and correct the documentation

@flsmith flsmith mentioned this pull request Dec 2, 2020
6 tasks
@flsmith flsmith force-pushed the translat-restricted branch 2 times, most recently from f77aa82 to 2f73bcd Compare December 3, 2020 15:13
Copy link
Collaborator

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

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

A few initial comments mostly about the documentation.

doc/translat.xml Outdated
gap> mat := [[G.1, G.2], [G.1, G.1], [G.2, G.3]];;
gap> S := ReesMatrixSemigroup(G, mat);;
gap> R := Range(RMSNormalization(S));;
gap> G := UnderlyingSemigroup(R);;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
gap> G := UnderlyingSemigroup(R);;

You can delete this line because normalising a RMS/RZMS doesn't change its underlying (semi)group. So here G is still defined as it was a few lines above.

#
gap> SEMIGROUPS.StartTest();

#T# RZMS Translational Hull
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the intervening years we've got rid of these #T# and #E# tags, so we may as well not reintroduce them now. So please just use #. 🙂 Same with the standard test file.

doc/z-chap14.xml Outdated
<Heading>
Translations and translational hulls
</Heading>
In this section we describe the functionality in Semigroups for working with
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
In this section we describe the functionality in Semigroups for working with
In this section we describe the functionality in &Semigroups; for working with

doc/translat.xml Outdated
<Func Name="RightTranslations" Arg="S"/>
<Func Name="LeftTranslationsSemigroup" Arg="S"/>
<Func Name="RightTranslationsSemigroup" Arg="S"/>
<Returns>The semigroup of left or right translations of the given semigroup</Returns>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<Returns>The semigroup of left or right translations of the given semigroup</Returns>
<Returns>A semigroup of left or right translations.</Returns>

doc/translat.xml Outdated
<Func Name="RightTranslation" Arg="R, map"/>
<Returns>A left or right translation.</Returns>
<Description>
For the semigroup of left of right translations and a Mapping on the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
For the semigroup of left of right translations and a Mapping on the
For the semigroup of left of right translations and a mapping on the

? Or do you deliberately want the capital letter for some reason?

doc/translat.xml Outdated
For the semigroup of left of right translations and a Mapping on the
underlying semigroup, or a transformation acting on the indices of the
canonical list of elements of the underlying semigroup,
LeftTranslation and RightTranslation return the corresponding translations.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
LeftTranslation and RightTranslation return the corresponding translations.
<C>LeftTranslation</C> and <C>RightTranslation</C> return the corresponding translations.

doc/translat.xml Outdated
Comment on lines 100 to 101
<Returns>The element of the translational hull corresponding to the given
left and right translations</Returns>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to point out, I could be wrong, but we usually use the <Returns> tag to say what kind of GAP object is returned, rather than its mathematical meaning. So this might be more in keeping as:

Suggested change
<Returns>The element of the translational hull corresponding to the given
left and right translations</Returns>
<Returns>A translational hull element.</Returns>

doc/translat.xml Outdated
<Filt Name="IsTranslationalHull" Type="synonym"/>
<Description>
<C>IsTranslationalHull</C> is a synonym for <C>IsSemigroup</C> and
<Ref Filt="IsBitranslationCollection"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<Ref Filt="IsBitranslationCollection"/>
<Ref Filt="IsBitranslationCollection"/>.

doc/translat.xml Outdated

<#GAPDoc Label="UnderlyingSemigroup">
<ManSection>
<Attr Name="UnderlyingSemigroup" Arg="S"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no <Returns> tag here.

##
# W rms-translat.gd
# Y Copyright (C) 2015-17 Finn Smith
##
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might want update the years here!

@flsmith flsmith force-pushed the translat-restricted branch 2 times, most recently from 3b08ac4 to d06bdcb Compare June 17, 2022 14:07
@james-d-mitchell james-d-mitchell added new-feature A label for PRs that contain new features major A label for issues or PRs that require a major amount of work or big changes. labels Jun 23, 2022
@flsmith
Copy link
Collaborator Author

flsmith commented Jul 13, 2022

The linting on this PR will fail until james-d-mitchell/gaplint#13 is merged/released (or some other solution).

@flsmith
Copy link
Collaborator Author

flsmith commented Jul 19, 2022

@james-d-mitchell I think this is ready to be looked at

Copy link
Collaborator

@james-d-mitchell james-d-mitchell left a comment

Choose a reason for hiding this comment

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

Nice one @flsmith! Subject to the comments in this review, I'm happy with this

gap/attributes/attr.gi Outdated Show resolved Hide resolved
#############################################################################
##

# The declarations in this file are purposefully undocumented.
Copy link
Collaborator

Choose a reason for hiding this comment

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

With what purpose?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They are internal, which I've noted in the file, but should I also preface them with SEMIGROUPS? I was avoiding doing so just because of how annoying that looks when they are used as filters.

gap/attributes/rms-translat.gi Outdated Show resolved Hide resolved
gap/attributes/rms-translat.gi Outdated Show resolved Hide resolved
gap/attributes/rms-translat.gi Outdated Show resolved Hide resolved
gap/attributes/translat.gd Outdated Show resolved Hide resolved
gap/attributes/translat.gd Outdated Show resolved Hide resolved
gap/attributes/translat.gd Outdated Show resolved Hide resolved
gap/attributes/translat.gd Outdated Show resolved Hide resolved
tst/extreme/translat.tst Outdated Show resolved Hide resolved
@james-d-mitchell james-d-mitchell added this to Todo in Next release via automation Sep 29, 2022
@james-d-mitchell james-d-mitchell moved this from Todo to In progress in Next release Sep 29, 2022
@james-d-mitchell james-d-mitchell merged commit 054cc00 into semigroups:main Sep 30, 2022
@james-d-mitchell james-d-mitchell moved this from In progress to Done in Next release Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major A label for issues or PRs that require a major amount of work or big changes. new-feature A label for PRs that contain new features
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants