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

Clarify documentation of GeneratorsOfDomain and add an example #3453

Merged
merged 1 commit into from
May 31, 2019

Conversation

ssiccha
Copy link
Contributor

@ssiccha ssiccha commented May 15, 2019

A domain comes with an associated operational structure. The documentation for GeneratorsOfDomain states:

For a domain D, GeneratorsOfDomain returns a list containing all elements of D,
perhaps with repetitions.

I'm changing this to:

For a domain D, GeneratorsOfDomain returns a list containing generators of D with respect to its operational structure.
The returned list may contain repetitions.

@ssiccha ssiccha added kind: bug Issues describing general bugs, and PRs fixing them topic: documentation Issues and PRs related to documentation release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels May 15, 2019
@coveralls
Copy link

coveralls commented May 15, 2019

Coverage Status

Coverage increased (+0.0006%) to 85.17% when pulling f14c871 on ssiccha:ss/doc-GeneratorsOfDomain into 87794e8 on gap-system:master.

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.

I don't think that's the intended meaning of GeneratorsOfDomain. Think about it: GeneratorsOfGroup returns the generators of an object viewed as a group, i.e., generators relative to the category of groups. For the same object, you can also call GeneratorsOfSemigroup or GeneratorsOfMagma, and they each give you larger sets. My interpretation thus is that GeneratorsOfDomain returns the most general set of generators, namely those relative to the category "Set".

@fingolfin
Copy link
Member

Of course we may now argue that this is not what it should be; but then the question is, what should it do? How do we decide which kind of "generators" are supposed to be returned? Those of a magma? A semigroup? A monoid? A group? A ring? A ring-with-one? etc. ... Or perhaps "The most specific one?" -- assuming that is always well-defined?

@ssiccha ssiccha added the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label May 18, 2019
@ssiccha
Copy link
Contributor Author

ssiccha commented May 18, 2019

Ah, I've interpreted the following section in the documentation as if there were a unique operational structure attached to a domain.

One important difference between the operational structure and the properties of
a  domain  is  that  the  operational  structure  is  fixed  when  the domain is
constructed,  whereas  properties  can  be discovered later.

But since domains are modelled via filters there can't be a unique "most specific with respect to inclusion" one.

In #3460 I suggest a category IsSetAsDomain. Would it make sense to add a documented synonym for GeneratorsOfDomain by the name of e.g. GeneratorsOfSetAsDomain?

Would it be problematic if installations of GeneratorsOfDomain assume another operational structure than the trivial one? Note that permutation groups don't have any method installed for GeneratorsOfDomain.

@ssiccha
Copy link
Contributor Author

ssiccha commented May 19, 2019

Note that permutation groups don't have any method installed for GeneratorsOfDomain. That's not true. Actually we have

gap> G := Group([(1,2), (3,4)]);
Group([ (1,2), (3,4) ])
gap> GeneratorsOfDomain(G);
[ (), (3,4), (1,2), (1,2)(3,4) ]

So GeneratorsOfDomain returns generators with respect to the trivial operational structure, that is it returns the domain as a set, as @fingolfin said. I will update my PR to clarify this in the documentation.

@ssiccha ssiccha force-pushed the ss/doc-GeneratorsOfDomain branch from 6d6f310 to 199b710 Compare May 21, 2019 14:11
@codecov
Copy link

codecov bot commented May 21, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@87794e8). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #3453   +/-   ##
=========================================
  Coverage          ?   85.34%           
=========================================
  Files             ?      699           
  Lines             ?   346465           
  Branches          ?        0           
=========================================
  Hits              ?   295675           
  Misses            ?    50790           
  Partials          ?        0
Impacted Files Coverage Δ
lib/domain.gd 100% <ø> (ø)

@ssiccha
Copy link
Contributor Author

ssiccha commented May 21, 2019

Nevermind my previous suggestions. I just updated this PR and I think the new version is a lot clearer.

@ssiccha ssiccha changed the title doc: fix doc of GeneratorsOfDomain Clarify documentation of GeneratorsOfDomain and add an example May 21, 2019
@ssiccha ssiccha force-pushed the ss/doc-GeneratorsOfDomain branch from 199b710 to f14c871 Compare May 21, 2019 14:12
@ssiccha ssiccha added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements and removed do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) kind: bug Issues describing general bugs, and PRs fixing them labels May 21, 2019
@fingolfin fingolfin merged commit 3e5ede2 into gap-system:master May 31, 2019
@ssiccha ssiccha deleted the ss/doc-GeneratorsOfDomain branch August 7, 2019 08:48
@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 21, 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
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 topic: documentation Issues and PRs related to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants