-
Notifications
You must be signed in to change notification settings - Fork 163
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 AsSemigroup
to reject non-associative inputs & declare IsCommutative
/IsAssociative
for IsCollection
#4373
Fix AsSemigroup
to reject non-associative inputs & declare IsCommutative
/IsAssociative
for IsCollection
#4373
Conversation
AsSemigroup
methodAsSemigroup
method
a7a788f
to
3227966
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Yes, the associativity test is necessary.
|
Thanks for the further information @ThomasBreuer. I'll investigate how that behaviour corresponds to the documentation, and try to add fixes to this PR as appropriate. |
@wilfwilson I have been thinking about installing a default |
20cdc58
to
eb0c8ea
Compare
I've now updated this along the lines you suggested @fingolfin, and removed the "do not merge" label. Let's see what the tests and code coverage say. |
eb0c8ea
to
cea9c29
Compare
@ThomasBreuer here's how things lie with this PR:
Shall we open a separate issue about this? |
cea9c29
to
011b6ce
Compare
AsSemigroup
methodAsSemigroup
& declare IsCommutative/IsAssociative
for IsMultiplicativeElementCollection
AsSemigroup
& declare IsCommutative/IsAssociative
for IsMultiplicativeElementCollection
AsSemigroup
& declare IsCommutative
/IsAssociative
for IsMultiplicativeElementCollection
This PR now includes a bugfix and an enhancement; I can break them up if you'd like (the bugfix would have to be based on top of the enhancement). As it stands, I wonder if such a combination will confuse the automated release notes creation script... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
84f5065
to
b1b7c93
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two details (one formulation and one changed requirement in a method installation) may cause problems, otherwise the changes are o.k.
In particular, I agree that we should open a new issue for collecting the problems with missing associativity checks in other domain constructions.
cfad551
to
22d917d
Compare
AsSemigroup
& declare IsCommutative
/IsAssociative
for IsMultiplicativeElementCollection
AsSemigroup
& declare IsCommutative
/IsAssociative
for IsCollection
22d917d
to
8afa55d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, apart from a suspicious formulation (for me as a non-native speaker).
8afa55d
to
c6d69b6
Compare
AsSemigroup
& declare IsCommutative
/IsAssociative
for IsCollection
AsSemigroup
to reject non-associative inputs & declare IsCommutative
/IsAssociative
for IsCollection
AsSemigroup
to reject non-associative inputs & declare IsCommutative
/IsAssociative
for IsCollection
AsSemigroup
to reject non-associative inputs & declare IsCommutative
/IsAssociative
for IsCollection
See #4030.
It might not be fast, but I think it is necessary.
I thought about adding a similar check to
SemigroupByGenerators
, but that (via the documentation forSemigroup
) is explicitly documented as not checking for associativity.Fixes #4030.
Text for release notes
AsSemigroup
, and correctly returnfail
for non-associative collections, such as non-associative magmas.IsCommutative
andIsAssociative
more generally forIsCollection
, rather than just forIsMagma
.(End of text for release notes)