Opened 10 years ago
Last modified 10 years ago
#13688 closed defect
FiniteEnumeratedSets cardinality override — at Version 5
Reported by: | Travis Scrimshaw | Owned by: | Sage Combinat CC user |
---|---|---|---|
Priority: | major | Milestone: | sage-5.8 |
Component: | categories | Keywords: | finite sets, category, cardinality, days45 |
Cc: | Nicolas M. Thiéry, Andrew Mathas, Sage Combinat CC user | Merged in: | |
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).
Change History (6)
comment:1 Changed 10 years ago by
Description: | modified (diff) |
---|
comment:2 Changed 10 years ago by
Authors: | → Travis Scrimshaw |
---|---|
Cc: | Nicolas M. Thiéry Andrew Mathas Sage Combinat CC user added |
Status: | new → needs_review |
Patch which has _cardinality_from_*
take dummy optional args and discard them.
comment:3 Changed 10 years ago by
Description: | modified (diff) |
---|
comment:4 follow-up: 5 Changed 10 years ago by
Keywords: | days45 added |
---|---|
Reviewers: | → 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 Changed 10 years ago by
Description: | modified (diff) |
---|---|
Status: | needs_review → 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 10 years ago by
Attachment: | trac_13688-finite_sets_cardinality_override-ts.patch added |
---|
Tweaked example to show we don't want to change the semantics of
cardinality()
.