Opened 8 years ago

Closed 8 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 nborie)

A general tool to define:

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)

trac_8519_Range_factory-nb.patch (24.7 KB) - added by nborie 8 years ago.
trac_8519_Range_factory-review-fh.patch (23.5 KB) - added by hivert 8 years ago.
trac_8519_Range_factory-review-fh.2.patch (21.7 KB) - added by hivert 8 years ago.
trac_8519_Range_factory-review2-nb.patch (1.6 KB) - added by nborie 8 years ago.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 8 years ago by nthiery

+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?

comment:2 Changed 8 years ago by nborie

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

  • Description modified (diff)

comment:4 Changed 8 years ago by nborie

  • Description modified (diff)

comment:5 follow-up: Changed 8 years ago by nborie

Hello Nicolas, will you set a +2 with the new description ?

comment:6 in reply to: ↑ 5 ; follow-up: Changed 8 years ago by nthiery

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 8 years ago by saliola

+1 for IntegerRange instead of Range.

comment:8 Changed 8 years ago by nborie

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 8 years ago by nborie

The contains function is false. It works only for IntegerRange?(-Infinity,Infinity,a,b)...

comment:10 follow-up: Changed 8 years ago by nborie

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 8 years ago by hivert

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 8 years ago by nborie

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

Changed 8 years ago by hivert

comment:13 Changed 8 years ago by hivert

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 8 years ago by hivert

I had a second though and decided that _from_integer_ was a little overkill. I removed it.

Cheers,

Florent

Changed 8 years ago by hivert

Changed 8 years ago by nborie

comment:15 Changed 8 years ago by nborie

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 8 years ago by hivert

  • Authors set to Nicolas Borie
  • Reviewers set to Florent Hivert
  • Status changed from needs_review to positive_review

Patch looks good, ready to go.

Florent

comment:17 follow-up: Changed 8 years ago by 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

Thanks for all. Nicolas.

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

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: Changed 8 years ago by 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)?

comment:20 in reply to: ↑ 19 Changed 8 years ago by nthiery

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 8 years ago by hivert

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 8 years ago by jhpalmieri

  • 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
Note: See TracTickets for help on using tickets.