Opened 10 years ago
Closed 9 years ago
#13688 closed defect (fixed)
FiniteEnumeratedSets cardinality override
Reported by: | tscrim | Owned by: | sage-combinat |
---|---|---|---|
Priority: | major | Milestone: | sage-5.8 |
Component: | categories | Keywords: | finite sets, category, cardinality, days45 |
Cc: | nthiery, andrew.mathas, sage-combinat | Merged in: | sage-5.8.beta1 |
Authors: | Travis Scrimshaw | Reviewers: | Andrew Mathas |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
Right now the category FiniteEnumeratedSets
overrides a parent class's cardinality()
when creating a list. Below is a minimal example of this behavior:
from sage.structure.parent import Parent from sage.categories.finite_enumerated_sets import FiniteEnumeratedSets class TestParent(Parent): def __init__(self): Parent.__init__(self, category=FiniteEnumeratedSets()) def __iter__(self): yield 1 return def cardinality(self, bad_arg): """ EXAMPLES:: sage: P = sage.combinat.category_doctest_fail.TestParent() sage: P.cardinality(-1) 1 sage: v = P.list(); v [1] sage: len(v) 1 sage: P.cardinality() 1 sage: P.cardinality(-1) # This test breaks 1 """ return 1 # we don't want to change the semantics of cardinality()
This seems to be caused by not checking if the parent class has a cardinality()
function implemented, and just overriding it with _cardinality_from_list()
(which takes no [optional/keyword] parameters).
Attachments (1)
Change History (7)
comment:1 Changed 10 years ago by
- Description modified (diff)
comment:2 Changed 10 years ago by
- Cc nthiery andrew.mathas sage-combinat added
- Status changed from new to needs_review
Patch which has _cardinality_from_*
take dummy optional args and discard them.
comment:3 Changed 9 years ago by
- Description modified (diff)
comment:4 follow-up: ↓ 5 Changed 9 years ago by
- Keywords days45 added
- Reviewers set to Andrew Mathas
The patch is quite innocuous and fixes a problem. All doc tests pass.
Can you add a doctest for testing for this? Presumably it showed up in a real class. Happy for you to set this to a positive review once that's done.
comment:5 in reply to: ↑ 4 Changed 9 years ago by
- Description modified (diff)
- Status changed from needs_review to positive_review
Replying to andrew.mathas:
Can you add a doctest for testing for this?
Done. I expanded on the example I gave in the description.
Presumably it showed up in a real class.
In partition.py
after #13605 is applied (of course without this patch).
Happy for you to set this to a positive review once that's done.
Also done. Thank you for reviewing this.
Best,
Travis
Changed 9 years ago by
comment:6 Changed 9 years ago by
- Merged in set to sage-5.8.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
Tweaked example to show we don't want to change the semantics of
cardinality()
.