Opened 11 years ago

Closed 10 years ago

#8925 closed enhancement (fixed)

__call__ for categories sets / enumeratedsets

Reported by: hivert Owned by: hivert
Priority: major Milestone: sage-4.6.2
Component: categories Keywords: Category call
Cc: Merged in: sage-4.6.2.alpha0
Authors: Florent Hivert Reviewers: Mike Hansen, Jason Grout
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

From sets_cat.py:

FIXME: the above behavior dates back from the first category
writeup. It is not consistent with :meth:`Category.__call__`.
Should we change it to just return ``ZZ`` instead?

Also EnumeratedSets().__call__(...) is missing.

Attachments (1)

trac_8925-call_set_enumset-fh.patch (5.7 KB) - added by hivert 10 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 11 years ago by hivert

  • Authors set to Florent Hivert
  • Status changed from new to needs_review

comment:2 follow-up: Changed 10 years ago by mhansen

  • Reviewers set to Mike Hansen
  • Status changed from needs_review to needs_work

Looks good except for some minor formatting issues. I put a review patch up on the patch server. Do you want to fold that in and reupload?

comment:3 in reply to: ↑ 2 Changed 10 years ago by hivert

  • Status changed from needs_work to needs_review

Replying to mhansen:

Looks good except for some minor formatting issues. I put a review patch up on the patch server. Do you want to fold that in and reupload?

Done ! I'm ok with your changes. Feel free to put a positive review if you are ok.

comment:4 follow-up: Changed 10 years ago by mhansen

  • Status changed from needs_review to positive_review

comment:5 Changed 10 years ago by jdemeyer

  • Milestone changed from sage-4.6.1 to sage-4.6.2

comment:6 in reply to: ↑ 4 Changed 10 years ago by hivert

Replying to mhansen:

Thanks for the review !

comment:7 follow-up: Changed 10 years ago by jason

Extremely picky, minor, minor thing, but these words: "For now, list's, tuple's, set's, Set's are coerced into finite" shouldn't have apostrophes in them, since the words are not possessive, but just plural.

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

  • Status changed from positive_review to needs_work

Replying to jason:

Extremely picky, minor, minor thing, but these words: "For now, list's, tuple's, set's, Set's are coerced into finite" shouldn't have apostrophes in them, since the words are not possessive, but just plural.

I reuploaded a new patche with the following diff folded:

diff --git a/sage/categories/enumerated_sets.py b/sage/categories/enumerated_sets.py
--- a/sage/categories/enumerated_sets.py
+++ b/sage/categories/enumerated_sets.py
@@ -103,7 +103,7 @@ class EnumeratedSets(Category):
             sage: EnumeratedSets()(Primes())
             Set of all prime numbers: 2, 3, 5, 7, ...
 
-        For now, list's, tuple's, set's, Set's are coerced into finite
+        For now, lists, tuples, sets, Sets are coerced into finite
         enumerated sets::
 
             sage: S = EnumeratedSets()([1, 2, 3]); S

So now you have to review it ;-)

Changed 10 years ago by hivert

comment:9 in reply to: ↑ 8 Changed 10 years ago by hivert

  • Status changed from needs_work to needs_review

Replying to hivert:

Replying to jason:

Extremely picky, minor, minor thing, but these words: "For now, list's, tuple's, set's, Set's are coerced into finite" shouldn't have apostrophes in them, since the words are not possessive, but just plural.

I reuploaded a new patche with the following diff folded:

Actually there where another occurrence in another file

diff --git a/trac_8925-call_set_enumset-fh.patch b/trac_8925-call_set_enumset-fh.patch
--- a/trac_8925-call_set_enumset-fh.patch
+++ b/trac_8925-call_set_enumset-fh.patch
@@ -83,7 +83,7 @@ diff --git a/sage/categories/finite_enum
 +            sage: FiniteEnumeratedSets()(Partitions(3)) # todo: not implemented: Partitions
 +            Partitions of 3
 +
-+        For now, list's, tuple's, set's, Set's are coerced into finite
++        For now, lists, tuples, sets, Sets are coerced into finite
 +        enumerated sets::
 +
 +            sage: FiniteEnumeratedSets()([1, 2, 3])

Again I folded this change and resubmitted the patch. Please review.

comment:10 Changed 10 years ago by mhansen

  • Reviewers changed from Mike Hansen to Mike Hansen, Jason Grout
  • Status changed from needs_review to positive_review

Looks good.

comment:11 Changed 10 years ago by jason

Looks good to me too. Thanks for working on even the most minor nitpicky requests :).

comment:12 Changed 10 years ago by jdemeyer

  • Merged in set to sage-4.6.2.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.