Opened 7 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 nthiery)

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)

trac_8579-category-magmas-nt.patch (35.4 KB) - added by nthiery 7 years ago.
This updated patch fixes the copyright headers

Download all attachments as: .zip

Change History (13)

comment:1 Changed 7 years ago by nthiery

  • Description modified (diff)
  • Keywords categories magma added
  • Status changed from new to needs_review

comment:2 Changed 7 years ago by rbeezer

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

comment:3 Changed 7 years ago by nthiery

  • Description modified (diff)

comment:4 Changed 7 years ago by rbeezer

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

Changed 7 years ago by nthiery

This updated patch fixes the copyright headers

comment:5 follow-up: Changed 7 years ago by rbeezer

  • 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 7 years ago by was

Finally, Magma gets included in Sage!!

comment:7 in reply to: ↑ 5 ; follow-up: Changed 7 years ago by hivert

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: Changed 7 years ago by nthiery

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.

comment:9 Changed 7 years ago by nthiery

  • Reviewers set to Robert Beezer, Florent Hivert

comment:10 in reply to: ↑ 8 ; follow-up: Changed 7 years ago by hivert

  • 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 rbeezer

Replying to hivert:

Then It's ok ! positive review.

Florent,

Thanks for finishing this one off!

Rob

comment:12 Changed 7 years ago by jhpalmieri

  • 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

Note: See TracTickets for help on using tickets.