Opened 8 years ago
Closed 7 years ago
#8579 closed defect (fixed)
Add the categories of magmas and additive magmas
Reported by: | nthiery | Owned by: | nthiery |
---|---|---|---|
Priority: | major | Milestone: | sage-4.4 |
Component: | categories | Keywords: | categories, magma |
Cc: | sage-combinat, roed | Merged in: | sage-4.4.alpha0 |
Authors: | Nicolas M. Thiéry | Reviewers: | Robert Beezer, Florent Hivert |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
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.
Attachments (1)
Change History (13)
comment:1 Changed 8 years ago by
- Description modified (diff)
- Keywords categories magma added
- Status changed from new to needs_review
comment:2 Changed 8 years ago by
comment:3 Changed 8 years ago by
- Description modified (diff)
comment:4 Changed 8 years ago by
Thanks, Nicolas, I'll test tonight with #7880.
comment:5 follow-up: ↓ 7 Changed 8 years ago by
- Cc roed added
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
comment:6 Changed 8 years ago by
Finally, Magma gets included in Sage!!
comment:7 in reply to: ↑ 5 ; follow-up: ↓ 8 Changed 7 years ago by
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
comment:8 in reply to: ↑ 7 ; follow-up: ↓ 10 Changed 7 years ago by
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 inSemigroups()
is sufficiently general to work in magmas (computing product by lifting/retracting). It should be moved inMagmas()
.
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.
comment:9 Changed 7 years ago by
- Reviewers set to Robert Beezer, Florent Hivert
comment:10 in reply to: ↑ 8 ; follow-up: ↓ 11 Changed 7 years ago by
- Status changed from needs_review to positive_review
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.
comment:11 in reply to: ↑ 10 Changed 7 years ago by
comment:12 Changed 7 years ago by
- Merged in set to sage-4.4.alpha0
- Resolution set to fixed
- Status changed from positive_review to closed
Merged "trac_8579-category-magmas-nt.patch" in 4.4.alpha0
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