Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#9056 closed enhancement (fixed)

Add the category of SemiRings with an example : NonNegativeIntegers()

Reported by: nborie Owned by: nborie
Priority: major Milestone: sage-4.4.4
Component: categories Keywords: semiring
Cc: sage-combinat, nthiery Merged in: sage-4.4.4.alpha1
Authors: Nicolas Borie Reviewers: Nicolas M. Thiéry
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by nborie)

All is in the title, we want :

sage: Semirings()
sage: Category of semirings
sage: NN = NonNegativeIntegerSemiring()
sage: NN.category()
Join of Category of semirings and Category of infinite enumerated sets

Attachments (3)

trac_9056_semirings_category-nb.2.patch (10.8 KB) - added by nborie 7 years ago.
trac_9056_semirings_category-nb.patch (15.8 KB) - added by nborie 7 years ago.
trac_9056-disable-llt-test.patch (1.4 KB) - added by mhansen 7 years ago.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 7 years ago by nborie

  • Description modified (diff)

comment:2 Changed 7 years ago by nborie

  • Description modified (diff)

comment:3 Changed 7 years ago by nborie

  • Status changed from new to needs_review

Changed 7 years ago by nborie

comment:4 Changed 7 years ago by nthiery

  • Authors set to Nicolas Borie
  • Milestone set to sage-4.4.3
  • Reviewers set to Nicolas M. Thiéry

I reviewed this patch while Nicolas was writting it. I'll run the tests shortly, and report.

Florent: please have a look to see if you agree with the concept, and if yes set a positive review.

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

  • Status changed from needs_review to needs_work

Hi Nicolas B.,

There are a couple failures in the category tests (due to the new test and new category); please fix them. Note that I made a small change to your patch, so you should make sure to first pull.

You might get a conflict with the primer improvement; in that case, move your patch just after it.

comment:6 in reply to: ↑ 5 Changed 7 years ago by nthiery

Replying to nthiery:

There are a couple failures in the category tests (due to the new test and new category); please fix them. Note that I made a small change to your patch, so you should make sure to first pull.

A couple failures in combinat/sf as well due to the new _test_distributivity.

comment:7 Changed 7 years ago by nborie

  • Status changed from needs_work to needs_review

As I did manage to run ALL TESTS on sagemath, I found files this patch affect... I didn't touch the primer (modified on combinat queue). This patch will have yo be updated if your primer improvements go in sage earlier.

I update the patch (without .2.patch, forget this one.)

Thank for your help for massive tests.

comment:8 Changed 7 years ago by nborie

Sorry and please wait, this local patch will break your queue Nicolas.

comment:9 Changed 7 years ago by nborie

This last update should be fine...

comment:10 Changed 7 years ago by nborie

One more update :

After advises form English speaker people, I change the name from NonNegativeIntegersSemiring? to NonNegativeIntegerSemiring?. See http://groups.google.com/group/sage-devel/browse_thread/thread/ffaf01ffb941078

I linked my module to the reference manual and fix a :class: ref in the doc thanks to Florent.

I am still ready for deeper review.

comment:11 follow-up: Changed 7 years ago by nborie

  • Description modified (diff)

comment:12 in reply to: ↑ 11 Changed 7 years ago by nthiery

Hi Nicolas,

Thanks for finalizing this!

I just pushed a small reviewer's patch on sage-combinat. Please review, and if ok fold and reupload here.

Changed 7 years ago by nborie

comment:13 Changed 7 years ago by nborie

  • Status changed from needs_review to positive_review

I am ok with your reviewer patch. I will try to delete ending blanks on my own since you made work well coloring with my hg qdiff. I qfolded your patch in mine and uploaded it...

Thanks for the review.

For the release manager ,apply only attachment:trac_9056_semirings_category-nb.patch

comment:14 Changed 7 years ago by nthiery

  • Status changed from positive_review to needs_work

I did not yet say to set a positive review on my behalf :-) Actually one test was failing in the primer. I am rerunning all tests.

comment:15 Changed 7 years ago by nthiery

  • Status changed from needs_work to needs_review

With my reviewer patch all test pass on Sage-4.4.3, with the following patches applied:

trac_8704-integer_range_print-fh.patch trac_9104_freemod_name-fix-nt.patch trac_8881-functorial_constructions-nt.patch trac_8742-lazy_format-fh.patch trac_8742-lazy_format-review-nt.patch trac_8930-enumerated_set_deprecate-fh.patch 8691_permutation_plainchange_tjb.patch trac_8926_family_repr-fh.patch trac_8902-subsets_call_fix-fh.patch trac_8888_partition_rim-fh.patch trac_8888_reviewer_jb.patch trac_8811_reduced_word_of_translations-nt.patch trac_8500_transitive_groups-final.patch trac_8549_cycle_enumerator-nb.patch trac_8490_square_free-vd.patch trac_9096_disj_union_sphinx_fix-fh.patch trac_8954-nilTemperley-as.patch trac_8913-cayley_graph_twosided_labels-nt.patch trac_8887-typo_monoid_prod-fh.patch trac_9106-UniqueRep_sphinx_fix-fh.patch trac_8876-triangular_morphisms_improve-fh.patch trac_8876-reviewer_patch-jb.patch sage-5.0.patch trac_9178-attrcall_hash_fix-nt.patch gap3_interface_v4.3.3.patch gap3_interface_patch2.patch trac_8747-testsuite-speedup-fh.patch trac_9056_semirings_category-nb.patch trac_9056_semirings_category-review-nt.patch

(note: actually interfaces/expect and interfaces/ecm failed, but those seem to be usual random failures on massena which would need to be investigated at some point).

Nicolas: please fold, and reupload, and set positive review on my behalf!

comment:16 Changed 7 years ago by nthiery

Arr, here is the list of patches in a more readable format:

trac_8704-integer_range_print-fh.patch
trac_9104_freemod_name-fix-nt.patch
trac_8881-functorial_constructions-nt.patch
trac_8742-lazy_format-fh.patch
trac_8742-lazy_format-review-nt.patch
trac_8930-enumerated_set_deprecate-fh.patch
8691_permutation_plainchange_tjb.patch
trac_8926_family_repr-fh.patch
trac_8902-subsets_call_fix-fh.patch
trac_8888_partition_rim-fh.patch
trac_8888_reviewer_jb.patch
trac_8811_reduced_word_of_translations-nt.patch
trac_8500_transitive_groups-final.patch
trac_8549_cycle_enumerator-nb.patch
trac_8490_square_free-vd.patch
trac_9096_disj_union_sphinx_fix-fh.patch
trac_8954-nilTemperley-as.patch
trac_8913-cayley_graph_twosided_labels-nt.patch
trac_8887-typo_monoid_prod-fh.patch
trac_9106-UniqueRep_sphinx_fix-fh.patch
trac_8876-triangular_morphisms_improve-fh.patch
trac_8876-reviewer_patch-jb.patch
sage-5.0.patch
trac_9178-attrcall_hash_fix-nt.patch
gap3_interface_v4.3.3.patch
gap3_interface_patch2.patch
trac_8747-testsuite-speedup-fh.patch
trac_9056_semirings_category-nb.patch
trac_9056_semirings_category-review-nt.patch

Changed 7 years ago by mhansen

comment:17 follow-up: Changed 7 years ago by mhansen

Things look good to me. Could I get a quick review of the patch I just posted. Without this, llt.py times out.

comment:18 in reply to: ↑ 17 Changed 7 years ago by ddrake

Replying to mhansen:

Things look good to me. Could I get a quick review of the patch I just posted. Without this, llt.py times out.

On my copy of 4.4.4.alpha0, with attachment:trac_9056_semirings_category-nb.patch applied, I got the timeout without your reviewer patch, and it takes about 8 seconds with your patch. I don't like disabling our doctest coverage, but this seems reasonable. Positive review to your reviewer patch. I'm leaving this as needs_review; Mike, you can change this if everything else is okay.

comment:19 follow-up: Changed 7 years ago by mhansen

  • Merged in set to sage-4.4.4.alpha1
  • Resolution set to fixed
  • Status changed from needs_review to closed

I've folded together the two #9056 patches on the combinat server together and merged the llt patch here.

comment:20 Changed 7 years ago by ddrake

Just as a little data point, on my machine, without Mike's patch, doctesting llt.py takes nearly 24 minutes (and passes!).

comment:21 follow-up: Changed 7 years ago by nborie

  • Cc nthiery added

Thanks Mike for the fix!

For now, I still don't really manage to integrate completely such patch which touch so many things in Sage. Dependencies are not trivial when you begin to modify categories.

For Nicolas Thiéry : This patch go in Sage before I fold your second reviewer patch :

diff --git a/sage/categories/primer.py b/sage/categories/primer.py
--- a/sage/categories/primer.py
+++ b/sage/categories/primer.py
@@ -122,6 +122,7 @@ Example of mathematical information::
          Category of rings,
          Category of rngs,
          Category of commutative additive groups,
+         Category of semirings,
          Category of commutative additive monoids,
          Category of commutative additive semigroups,
          Category of additive magmas,
@@ -503,6 +504,7 @@ This gives the following order::
      Category of algebras over Rational Field,
      Category of rings,
      Category of rngs,
+     Category of semirings,
      Category of monoids,
      Category of semigroups,
      Category of magmas,

I don't no the status about your improvements of the category primer but be aware about this fact. As I don't want to produce some chaos in the queue, I didn't touch your reviewer patch "trac_9056_semirings_category-review-nt.patch".

Sorry to being late to fold it.

Nicolas (the little).

comment:22 in reply to: ↑ 21 Changed 7 years ago by nthiery

Replying to nborie:

For now, I still don't really manage to integrate completely such patch which touch so many things in Sage. Dependencies are not trivial when you begin to modify categories.

Don't worry, you are doing a great job.

For Nicolas Thiéry : This patch go in Sage before I fold your second reviewer patch :

Mike said above that he took the two patches right away from the Sage-Combinat queue and folded them together. So all is fine.

comment:23 in reply to: ↑ 19 Changed 7 years ago by nthiery

Replying to mhansen:

I've folded together the two #9056 patches on the combinat server together and merged the llt patch here.

Thanks Mike!

For later reference, do you mind uploading here the exact patches that you merged, if you still have them under hand?

Note: See TracTickets for help on using tickets.