Opened 9 years ago

Closed 4 years ago

#13979 closed defect (fixed)

Make CartesianProduct_iter a proper Parent

Reported by: cnassau Owned by: sage-combinat
Priority: major Milestone: sage-8.2
Component: combinatorics Keywords:
Cc: vdelecroix, tscrim Merged in:
Authors: Christian Nassau, Frédéric Chapoton Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: dd38671 (Commits, GitHub, GitLab) Commit: dd386711b472cb5ac2250c5bce8d1492aae6a8c3
Dependencies: Stopgaps:

Status badges

Description (last modified by rws)

In Sage 5.7.beta4 the class CartesianProduct_iter is still a CombinatorialClass.

Previous ticket description:


Furthermore, the cartesian product can't handle infinite sets as factors. As a consequence the following is currently broken:

sage: M=CombinatorialFreeModule(GF(3),Integers())
sage: N=tensor((M,))
sage: cartesian_product((N,N))
Traceback (most recent call last):
...
TypeError: len() of unsized object

The attached patch

  • derives CartesianProduct_iter from EnumeratedSetFromIterator instead
  • adds doctests to verify that the TestSuites? work
  • makes sure that infinite factors are properly handled

=========

16 months later the test passes - must have been fixed by some other ticket

Attachments (1)

cna13979.patch (10.9 KB) - added by cnassau 9 years ago.

Download all attachments as: .zip

Change History (31)

comment:1 Changed 9 years ago by cnassau

  • Status changed from new to needs_review

comment:2 Changed 9 years ago by cnassau

  • Authors set to Christian Nassau
  • Description modified (diff)

comment:3 Changed 9 years ago by cnassau

  • Description modified (diff)

comment:4 Changed 9 years ago by cnassau

  • Description modified (diff)
  • Summary changed from CartesianProduct.cardinality can't handle infinite sets to Make CartesianProduct_iter a proper Parent

comment:5 Changed 9 years ago by cnassau

  • Status changed from needs_review to needs_work

comment:6 Changed 9 years ago by cnassau

  • Status changed from needs_work to needs_review

Changed 9 years ago by cnassau

comment:7 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:8 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:9 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:10 Changed 8 years ago by rws

  • Status changed from needs_review to needs_work

Patch does not merge.

comment:11 Changed 8 years ago by cnassau

  • Description modified (diff)
  • Milestone changed from sage-6.3 to sage-duplicate/invalid/wontfix

comment:12 Changed 8 years ago by rws

  • Description modified (diff)
  • Milestone changed from sage-duplicate/invalid/wontfix to sage-6.3

Still, your patch contains the categorification of class CartesianProduct_iters and doctests, which would be valuable to have in Sage. Thus, I modified the description accordingly. As to wontfix, you're not required to follow the ticket, others may want to take over. Unfortunately there's no way to unsubscribe from the mailing, as far as I know.

comment:13 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:14 follow-up: Changed 4 years ago by chapoton

  • Branch set to u/chapoton/13979
  • Cc vdelecroix tscrim added
  • Commit set to 88c38d9ebbaa39441795d648dcdef78c86c90c68

Here is a minimal fix

but CartesianProduct? was deprecated in #18411 and should rather be fully removed


New commits:

88c38d9trac 13979 changes in CartesianProduct

comment:15 Changed 4 years ago by chapoton

  • Status changed from needs_work to needs_info

comment:16 Changed 4 years ago by chapoton

  • Milestone changed from sage-6.4 to sage-8.2

comment:17 in reply to: ↑ 14 Changed 4 years ago by tscrim

Replying to chapoton:

but CartesianProduct was deprecated in #18411 and should rather be fully removed

There is a nontrivial use of CartesianProduct_iters in CombinatorialFreeModule_Tensor (which in turn, calls MapCombinatorialClass), so we cannot remove it outright currently.

comment:18 follow-up: Changed 4 years ago by chapoton

  • Authors changed from Christian Nassau to Christian Nassau, Frédéric Chapoton
  • Status changed from needs_info to needs_review

So, is my branch an useful step after all ?

comment:19 Changed 4 years ago by chapoton

  • Status changed from needs_review to needs_work

some failing doctests

comment:20 in reply to: ↑ 18 Changed 4 years ago by tscrim

Replying to chapoton:

So, is my branch an useful step after all ?

Yes, I think it is.

comment:21 Changed 4 years ago by chapoton

So what should we do with the from sage.sets.set_from_iterator import EnumeratedSetFromIterator that add something to the global namespace ?

comment:22 Changed 4 years ago by tscrim

It adds it to the global namespace? It just looks like it adds it to the startup, which is a necessary evil here I think.

comment:23 Changed 4 years ago by git

  • Commit changed from 88c38d9ebbaa39441795d648dcdef78c86c90c68 to 0c41d46d6c8dfc32ed1858db5ea38a3c2529f883

Branch pushed to git repo; I updated commit sha1. New commits:

0c41d46trac 13979 fixing doctests in dev_tools

comment:24 Changed 4 years ago by chapoton

  • Status changed from needs_work to needs_review

indeed, you are right. I have changed the failing doctest to use another class.

comment:25 Changed 4 years ago by tscrim

I'm wondering actually if we should change that test to do something that loads multiple modules so it is clear that it does has more side-effects (it looks in a way like it loads the specific module you are after in that test). Although I don't particularly care what we do, but I would like to see what your opinion is.

comment:26 Changed 4 years ago by chapoton

Why not. But I have no idea of where to look for a better example...

comment:27 Changed 4 years ago by tscrim

  • Branch changed from u/chapoton/13979 to u/tscrim/13979
  • Commit changed from 0c41d46d6c8dfc32ed1858db5ea38a3c2529f883 to dd386711b472cb5ac2250c5bce8d1492aae6a8c3
  • Reviewers set to Travis Scrimshaw

Sorry it took a little bit. Here is an example using sage.monoids.


New commits:

dd38671Changing test in dev_tools.py.

comment:28 Changed 4 years ago by chapoton

ok, good to go for me. Travis, please confirm.

comment:29 Changed 4 years ago by tscrim

  • Status changed from needs_review to positive_review

Yep. Thanks.

comment:30 Changed 4 years ago by vbraun

  • Branch changed from u/tscrim/13979 to dd386711b472cb5ac2250c5bce8d1492aae6a8c3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.