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 the categories of magmas and additive magmas #8579

Closed
nthiery opened this issue Mar 22, 2010 · 15 comments
Closed

Add the categories of magmas and additive magmas #8579

nthiery opened this issue Mar 22, 2010 · 15 comments

Comments

@nthiery
Copy link
Contributor

nthiery commented Mar 22, 2010

This patch adds the categories Magmas() and AdditiveMagmas()
(sets with a plain binary operation * or +).

It factors out some of the code previously in Semigroups / CommutativeAdditiveSemigroups.

This is used by the updated #7555 to make it more general.

Depends trivially on #7880.

CC: @sagetrac-sage-combinat @roed314

Component: categories

Keywords: magma

Author: Nicolas M. Thiéry

Reviewer: Robert Beezer, Florent Hivert

Merged: sage-4.4.alpha0

Issue created by migration from https://trac.sagemath.org/ticket/8579

@nthiery nthiery added this to the sage-4.4 milestone Mar 22, 2010
@nthiery nthiery self-assigned this Mar 22, 2010
@nthiery
Copy link
Contributor Author

nthiery commented Mar 22, 2010

Changed keywords from none to magma

@nthiery

This comment has been minimized.

@rbeezer
Copy link
Mannequin

rbeezer mannequin commented Mar 23, 2010

comment:2

Hi Nicolas,

I get about 10 "hunk failures" trying to apply this to a stock 4.3.4.

I noticed a "-git" in the diff commands in the patch, which I don't see in the patches I usually make. Is it it me, or does this patch need some attention?

Rob

@nthiery

This comment has been minimized.

@rbeezer
Copy link
Mannequin

rbeezer mannequin commented Mar 23, 2010

comment:4

Thanks, Nicolas, I'll test tonight with #7880.

@nthiery
Copy link
Contributor Author

nthiery commented Mar 24, 2010

Attachment: trac_8579-category-magmas-nt.patch.gz

This updated patch fixes the copyright headers

@rbeezer
Copy link
Mannequin

rbeezer mannequin commented Mar 24, 2010

comment:5

This passes all tests on 4.3.4.final and docs build without any problems. It also works well when the fairly extensive #7555 is applied on top of it. So there should be no surprises for the interested reviewer.

I can't say at the moment that I know enough about implementing categories to verify its completeness, so right now it either (a) needs a quick review from a knowledgeable category specialist, or (b) I need to get more education on categories so #7555 can go in.

Rob

@williamstein
Copy link
Contributor

comment:6

Finally, Magma gets included in Sage!!

@hivert
Copy link
Contributor

hivert commented Mar 31, 2010

comment:7

Hi Rob,

Replying to @rbeezer:

This passes all tests on 4.3.4.final and docs build without any problems. It also works well when the fairly extensive #7555 is applied on top of it. So there should be no surprises for the interested reviewer.

Thanks for checking this.

I can't say at the moment that I know enough about implementing categories to verify its completeness, so right now it either (a) needs a quick review from a knowledgeable category specialist, or (b) I need to get more education on categories so #7555 can go in.

From the category implementation point of view, everything looks fine upto the
following small problem which can be left for a future patch: the
SubQuotient code in Semigroups() is sufficiently general to work
in magmas (computing product by lifting/retracting). It should be moved in
Magmas().

Otherwise, I'm okay to give a positive review. I'll ask Nicolas directly.

Florent

@nthiery
Copy link
Contributor Author

nthiery commented Apr 1, 2010

comment:8

Replying to @hivert:

From the category implementation point of view, everything looks fine upto the
following small problem which can be left for a future patch: the
SubQuotient code in Semigroups() is sufficiently general to work
in magmas (computing product by lifting/retracting). It should be moved in
Magmas().

Ah, right, I forgot to mention that. I decided to implement this moving in the upcoming functorial construction patch instead, in order to avoid potential conflicts between the two patches.

@nthiery
Copy link
Contributor Author

nthiery commented Apr 1, 2010

Reviewer: Robert Beezer, Florent Hivert

@hivert
Copy link
Contributor

hivert commented Apr 1, 2010

comment:10

Replying to @nthiery:

Ah, right, I forgot to mention that. I decided to implement this moving in the upcoming functorial construction patch instead, in order to avoid potential conflicts between the two patches.

Then It's ok ! positive review.

@rbeezer
Copy link
Mannequin

rbeezer mannequin commented Apr 1, 2010

comment:11

Replying to @hivert:

Then It's ok ! positive review.

Florent,

Thanks for finishing this one off!

Rob

@jhpalmieri
Copy link
Member

Merged: sage-4.4.alpha0

@jhpalmieri
Copy link
Member

comment:12

Merged "trac_8579-category-magmas-nt.patch" in 4.4.alpha0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants