Opened 8 years ago

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

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)

trac_13688-finite_sets_cardinality_override-ts.patch (3.5 KB) - added by tscrim 8 years ago.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 8 years ago by tscrim

  • Description modified (diff)

Tweaked example to show we don't want to change the semantics of cardinality().

comment:2 Changed 8 years ago by tscrim

  • Authors set to Travis Scrimshaw
  • 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 8 years ago by tscrim

  • Description modified (diff)

comment:4 follow-up: Changed 8 years ago by andrew.mathas

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

  • 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

comment:6 Changed 8 years ago by jdemeyer

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