#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 )
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)
Change History (26)
comment:1 Changed 7 years ago by
- Description modified (diff)
comment:2 Changed 7 years ago by
- Description modified (diff)
comment:3 Changed 7 years ago by
- Status changed from new to needs_review
Changed 7 years ago by
comment:4 Changed 7 years ago by
- Milestone set to sage-4.4.3
- Reviewers set to Nicolas M. Thiéry
comment:5 follow-up: ↓ 6 Changed 7 years ago by
- 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
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
- 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
Sorry and please wait, this local patch will break your queue Nicolas.
comment:9 Changed 7 years ago by
This last update should be fine...
comment:10 Changed 7 years ago by
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: ↓ 12 Changed 7 years ago by
- Description modified (diff)
comment:12 in reply to: ↑ 11 Changed 7 years ago by
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
comment:13 Changed 7 years ago by
- 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
- 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
- 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
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
comment:17 follow-up: ↓ 18 Changed 7 years ago by
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
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: ↓ 23 Changed 7 years ago by
- 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
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: ↓ 22 Changed 7 years ago by
- 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
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.
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.