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 argument checking in FreeSemigroup, prevent creation of inconsistent objects #4372

Merged
merged 1 commit into from
Apr 21, 2021

Conversation

wilfwilson
Copy link
Member

@wilfwilson wilfwilson commented Apr 8, 2021

See #1385.

It was previously possible to use FreeSemigroup to create a seemingly free semigroup with zero generators (rank zero), but this object believed itself to be infinite, and to contain elements (such a semigroup would be empty). Note that free semigroups of rank zero are not documented as being supported.

Some other ways that one might want to create free semigroups of rank zero led to unhelpful error messages, and in general, the argument checking of FreeSemigroup was not very thorough and gave sometimes unhelpful error messages.

In this commit, I overhaul the way that FreeSemigroup checks its arguments, in particular always disallowing a free semigroup of rank zero, and overall making the checking more robust, and making the error messages more descriptive.

This addresses the bugs reported in issue #1385, although it does not address the feature request in that issue to support free semigroups of rank zero.

I also took the opportunity to fix a typo in the documentation for FreeSemigroup, and to make a clarification. Further improvements could be made to this documentation, but I leave that to the future.

Description

Text for release notes

Fix bugs caused when calling FreeSemigroup with incorrect arguments.

(End of text for release notes)

@wilfwilson wilfwilson added kind: bug Issues describing general bugs, and PRs fixing them kind: bug: wrong result Issues describing bugs that result in mathematically or otherwise wrong results, and PRs fixing them kind: bug: unexpected error Issues describing bugs in which computation unexpectedly encounters an error, and PRs fixing them topic: library release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Apr 8, 2021
@wilfwilson wilfwilson force-pushed the free-semigroup branch 3 times, most recently from 25f472e to c801433 Compare April 8, 2021 12:04
@wilfwilson wilfwilson force-pushed the free-semigroup branch 5 times, most recently from 0e0c8c7 to a83c443 Compare April 10, 2021 23:28
See gap-system#1385

It was previously possible to use `FreeSemigroup` to create a
seemingly free semigroup with zero generators (rank zero), but
this object believed itself to be infinite, and to contain elements
(such a semigroup would be empty). Note that free semigroups of rank
zero are not documented as being supported.

Some other ways that one might want to create free semigroups of
rank zero led to unhelpful error messages, and in general, the
argument checking of `FreeSemigroup` was not very thorough and gave
sometimes unhelpful error messages.

In this commit, I overhaul the way that `FreeSemigroup` checks its
arguments, in particular always disallowing a free semigroup of rank
zero, and overall making the checking more robust, and making the error
messages more descriptive.

This addresses the _bugs_ reported in issue gap-system#1385, although it does not
address the _feature request_ in that issue to support free semigroups of
rank zero.

I also took the opportunity to fix a typo in the documentation
for `FreeSemigroup`, and to make a clarification. Further improvements
could be made to this documentation, but I leave that to the future.
@fingolfin
Copy link
Member

Just to verify: issue #1385 also mentions FreeMagma, but I think this PR does nothing about that, right?

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.

Looks good to me, thanks!

@wilfwilson wilfwilson added the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Apr 11, 2021
@wilfwilson
Copy link
Member Author

wilfwilson commented Apr 11, 2021

Correct, this PR doesn't do anything about FreeMagma.

In my opinion, #1385 is about two things: a wrong-result bug report (FreeSemigroup([]) giving a semigroup of size infinity) and a feature request (FreeSemigroup and FreeMagma should support the creation of free semigroups and magmas with zero generators).

The primary purpose of this PR was to deal with the wrong-result bug in preparation for GAP 4.12.0. Given the way that the issue was written, I assumed that the wrong-result bug only applied FreeSemigroup. But it applies to FreeMagma too (and possibly other such functions).

I'll have a think about whether I want to do to address this too, and if so how. I don't want this to snowball 🙂!

The code for FreeMagma (I assume) should probably be an almost-duplicate of the code for FreeSemigroup (and for FreeMagmaWithOne? and FreeMonoid?). But I think it would be silly to duplicate so much technical code. So perhaps I'll refactor them to combine them.

I will probably create a new issue, as a feature request for supporting these things on zero generators – unless it turns out that it makes much more sense to just combine all those changes in this one PR.

@fingolfin
Copy link
Member

This PR has a "do not merge" label. So, what is left to be done?

@wilfwilson
Copy link
Member Author

#4372 (comment):

I'll have a think about whether I want to do to address this too, and if so how.

I wanted to decide whether this PR should also deal with FreeMagma (and FreeMonoid and FreeMagmaWithOne). But I think I've just decided now that I'll deal with those in a separate PR.

@wilfwilson wilfwilson removed the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Apr 21, 2021
@wilfwilson wilfwilson merged commit 1af8f3a into gap-system:master Apr 21, 2021
@wilfwilson wilfwilson deleted the free-semigroup branch April 21, 2021 10:07
@fingolfin fingolfin added release notes: use title For PRs: the title of this PR is suitable for direct use 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 17, 2022
@fingolfin fingolfin changed the title Check arguments to FreeSemigroup more thoroughly Fix argument checking in FreeSemigroup, prevent creation of inconsistent objects Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug: unexpected error Issues describing bugs in which computation unexpectedly encounters an error, and PRs fixing them kind: bug: wrong result Issues describing bugs that result in mathematically or otherwise wrong results, and PRs fixing them kind: bug Issues describing general bugs, and PRs fixing them release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants