Opened 7 months ago

Closed 6 months ago

#15195 closed defect (fixed)

Family cardinality does not catch enough errors

Reported by: tscrim Owned by: tscrim
Priority: major Milestone: sage-5.13
Component: combinatorics Keywords: Family cardinality
Cc: nthiery Merged in: sage-5.13.beta2
Authors: Travis Scrimshaw Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by tscrim)

We have the following:

sage: C = CartesianProduct(PositiveIntegers(), [1,2,3])
sage: C.cardinality()
+Infinity
sage: F = Family(C, lambda x: x)
sage: F.cardinality()
Throws a TypeError

The problem is that the cardinality() tries to run len() on an infinite set, which throws a TypeError. Fixes this by also catching type errors.

Attachments (1)

trac_15195-family_cardinality-ts.patch (1.1 KB) - added by tscrim 7 months ago.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 7 months ago by tscrim

  • Description modified (diff)
  • Status changed from new to needs_review

Changed 7 months ago by tscrim

comment:2 Changed 6 months ago by ncohen

Hmmm... It fixes the bug indeed, but why wouldn't we do the opposite and first try to call .set.cardinality() and on AttributeError run the len version ? Is this a speed problem ?

Nathann

comment:3 Changed 6 months ago by tscrim

  • Cc nthiery added

Its hard for me to say. If it typically gets python objects like list, then it will be faster. If it's expecting objects with a cardinality() method, then it's the reverse.

Nicolas, which do you think should be attempted first (note that this is for LazyFamily)?

Best,
Travis

comment:4 Changed 6 months ago by ncohen

  • Reviewers set to Nathann Cohen
  • Status changed from needs_review to positive_review

Okay okay, if it's about comparing the different use-cases then let's merge this patch first then think about that.

Especially if obtaining an answer takes three days :-P

Nathann

comment:5 Changed 6 months ago by tscrim

Thanks Nathann.

Travis

comment:6 Changed 6 months ago by jdemeyer

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