Opened 7 years ago
Closed 7 years ago
#8519 closed enhancement (fixed)
Add a factory of finite/infinite enumerated set (with categories) defines by Range(begin, end, step)
Reported by: | nborie | Owned by: | nborie |
---|---|---|---|
Priority: | major | Milestone: | sage-4.4 |
Component: | combinatorics | Keywords: | range, categories, set, integer |
Cc: | sage-combinat | Merged in: | sage-4.4.alpha0 |
Authors: | Nicolas Borie | Reviewers: | Florent Hivert |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
A general tool to define:
- PositiveInteger?
- NonNegativeInteger?
- More.....
The idea is : Range(begin, end, step) returns a proper finite/infinite set with categories of the defined set as you think...
(12:37:41) hivert: For example but not only: Range(5, 21321312, 3)...
(12:38:09) hivert: Range(0, Infinity, 1) <=> NonNegativeIntegers?
(12:38:18) hivert: Range(1, Infinity, 1) <=> PositiveIntegers?
Attachments (4)
Change History (26)
comment:1 Changed 7 years ago by
comment:2 Changed 7 years ago by
- Description modified (diff)
- Keywords range categories set added; positive removed
- Summary changed from Add a set of Positive Integer with categories (and factor the code with NonNegativeInteger) to Add a factory of finite/infinite enumerated set (with categories) defines by Range(begin, end, step)
comment:3 Changed 7 years ago by
- Description modified (diff)
comment:4 Changed 7 years ago by
- Description modified (diff)
comment:5 follow-up: ↓ 6 Changed 7 years ago by
Hello Nicolas, will you set a +2 with the new description ?
comment:6 in reply to: ↑ 5 ; follow-up: ↓ 7 Changed 7 years ago by
Replying to nborie:
Hello Nicolas, will you set a +2 with the new description ?
Yes. I just spent 5 minutes discussing about this with Florent over the phone. In the end, we vote for IntegerRange?(a,b, step=c), and NonNegativeIntegers? / PositiveIntegers? being subclasses (to add further properties; like the fact that they are semigroups/monoids/semirings/....).
comment:7 in reply to: ↑ 6 Changed 7 years ago by
+1 for IntegerRange
instead of Range
.
comment:8 Changed 7 years ago by
So guys,
I add a patch with a first implementation...
Questions:
- First, if you find a language mistake, please tell me.
- What should I keep, what should I drop in NonNegativeIntegers? ?
- Does IntegerRange? need an _element_constructor_ method ?
- What should I had in PositiveIntegers? (easy thing please) ?
- Do you want to tell me about your day ?
- Any comment ....
Thanks for further advises.
comment:9 Changed 7 years ago by
The contains function is false. It works only for IntegerRange?(-Infinity,Infinity,a,b)...
comment:10 follow-up: ↓ 11 Changed 7 years ago by
Ok, after think a while...
I will update this code and give IntegerRange? the behavior of range() if and only if someone (else than me) can give me an empty set with categories which can pass the tests....
sage: TestSuite(Set([])).run() The following tests failed: _test_an_element, _test_category, _test_elements, _test_some_elements
I need this feature for coherence!! Python range easily give out an empty list like:
sage: range(1,20,-1) [] sage: range(20,1,1) []
Adding - and + Infinity, we will have a lot of arguments which will build an emptyset. Currently, I did not find a good empty set with categories.
My code does not allow negative step
and check that
begin
<
end
to avoid the empty case.
On other hand, my current status is: Benchmarks for my PhD Thesis --> add number_of_transitive_group --> Add enumerated set of TransitiveGroups?() --> Add PositiveInteger? --> Add IntegerRange? feature --> MAX DEPTH OF RECURSION...
As this EmptySet? (with categories) is an empty thing, I have an idea of who can implemented that ?
comment:11 in reply to: ↑ 10 Changed 7 years ago by
Replying to nborie:
I will update this code and give IntegerRange? the behavior of range() if and only if someone (else than me) can give me an empty set with categories which can pass the tests....
sage: TestSuite(Set([])).run() The following tests failed: _test_an_element, _test_category, _test_elements, _test_some_elements
For consintency of the category you should use an EnumeratedSet
rather than a Set
:
sage: f = FiniteEnumeratedSet([]) sage: TestSuite(f).run() [...] The following tests failed: _test_an_element, _test_elements, _test_some_elements
You can't expect to get anything better than that with the current specification of sets: All those three tests are buggy in the sense that they suppose that there is at least one element in the set. So forget about it and don't run TestSuite? on an empty set until we fix this.
comment:12 Changed 7 years ago by
- Status changed from new to needs_review
This last version begin to be final...
I didn't manage to attach PositiveInteger? to the reference manual. The docbuild was always saying : UNABLE TO FIND THE MODULE. Even after 3421 checks, I didn't succeed...
Changed 7 years ago by
Changed 7 years ago by
comment:13 Changed 7 years ago by
Hi Nicolas,
I added a review patch which
- Adds a class factory
- Adds a cardinality method
- Improves the doc
- temporarily solve the handling of empty sets
Please review it,
Except for those issues your patch is good,
Florent
comment:14 Changed 7 years ago by
I had a second though and decided that _from_integer_
was a little overkill. I removed it.
Cheers,
Florent
Changed 7 years ago by
Changed 7 years ago by
comment:15 Changed 7 years ago by
Hi Florent,
I am very Ok with your changes! To complete this review, please check the 3 hyperlinks I just fixed in positive_integers.py and non_negative_integers.py. That's will be a very easy review I think.
Patchs to be applied :
trac_8519_Range_factory-nb.patch trac_8519_Range_factory-review-fh.2.patch trac_8519_Range_factory-review2-nb.patch
Thanks for all.
comment:16 Changed 7 years ago by
- Reviewers set to Florent Hivert
- Status changed from needs_review to positive_review
Patch looks good, ready to go.
Florent
comment:17 follow-up: ↓ 18 Changed 7 years ago by
My last comment was not well formatted, so for the release manager, please apply :
- trac_8519_Range_factory-nb.patch
- trac_8519_Range_factory-review-fh.2.patch
- trac_8519_Range_factory-review2-nb.patch
Thanks for all. Nicolas.
comment:18 in reply to: ↑ 17 ; follow-up: ↓ 19 Changed 7 years ago by
Replying to nborie:
My last comment was not well formatted, so for the release manager, please apply :
- trac_8519_Range_factory-nb.patch
- trac_8519_Range_factory-review-fh.2.patch
- trac_8519_Range_factory-review2-nb.patch
Reviewing this I noticed that we forgot to mention that this depends on #8524
Florent
comment:19 in reply to: ↑ 18 ; follow-ups: ↓ 20 ↓ 21 Changed 7 years ago by
Replying to hivert:
Reviewing this I noticed that we forgot to mention that this depends on #8524
In what way does it depend on it? I've merged the three patches into a prototype for Sage 4.4.alpha0, and all tests pass. Do I need to back that change out now? Or can someone review #8524 quickly (and thoroughly)?
comment:20 in reply to: ↑ 19 Changed 7 years ago by
Replying to jhpalmieri:
Replying to hivert:
Reviewing this I noticed that we forgot to mention that this depends on #8524
In what way does it depend on it? I've merged the three patches into a prototype for Sage 4.4.alpha0, and all tests pass. Do I need to back that change out now? Or can someone review #8524 quickly (and thoroughly)?
Done (well, almost; Florent just need to check my changes). The dependency is functional, not syntactical. I would have expected the tests to fail without #8524! Maybe the problem that was encountered and triggered the writting of #8524 did not end up being doctested, which would be bad!
comment:21 in reply to: ↑ 19 Changed 7 years ago by
Replying to jhpalmieri:
Replying to hivert:
Reviewing this I noticed that we forgot to mention that this depends on #8524
In what way does it depend on it? I've merged the three patches into a prototype for Sage 4.4.alpha0, and all tests pass. Do I need to back that change out now? Or can someone review #8524 quickly (and thoroughly)?
My mistake ! Forget about my comment.
comment:22 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 in 4.4.alpha0:
- trac_8519_Range_factory-nb.patch
- trac_8519_Range_factory-review-fh.2.patch
- trac_8519_Range_factory-review2-nb.patch
+1!
That will be useful for NCSF too (see the beginning of sage/combinat/ncsf/categories.py).
Just mind the 's': PositiveIntegers?
Let me guess: TransitiveGroup?(0,1) does not work, right? Maybe it should actually?