Opened 10 years ago
Closed 5 years ago
#13979 closed defect (fixed)
Make CartesianProduct_iter a proper Parent
Reported by:  Christian Nassau  Owned by:  Sage Combinat CC user 

Priority:  major  Milestone:  sage8.2 
Component:  combinatorics  Keywords:  
Cc:  Vincent Delecroix, Travis Scrimshaw  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: 
Description (last modified by )
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
fromEnumeratedSetFromIterator
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)
Change History (31)
comment:1 Changed 10 years ago by
Status:  new → needs_review 

comment:2 Changed 10 years ago by
Authors:  → Christian Nassau 

Description:  modified (diff) 
comment:3 Changed 10 years ago by
Description:  modified (diff) 

comment:4 Changed 10 years ago by
Description:  modified (diff) 

Summary:  CartesianProduct.cardinality can't handle infinite sets → Make CartesianProduct_iter a proper Parent 
comment:5 Changed 10 years ago by
Status:  needs_review → needs_work 

comment:6 Changed 10 years ago by
Status:  needs_work → needs_review 

Changed 10 years ago by
Attachment:  cna13979.patch added 

comment:7 Changed 9 years ago by
Milestone:  sage5.11 → sage5.12 

comment:8 Changed 9 years ago by
Milestone:  sage6.1 → sage6.2 

comment:9 Changed 9 years ago by
Milestone:  sage6.2 → sage6.3 

comment:10 Changed 9 years ago by
Status:  needs_review → needs_work 

comment:11 Changed 9 years ago by
Description:  modified (diff) 

Milestone:  sage6.3 → sageduplicate/invalid/wontfix 
comment:12 Changed 9 years ago by
Description:  modified (diff) 

Milestone:  sageduplicate/invalid/wontfix → sage6.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 8 years ago by
Milestone:  sage6.3 → sage6.4 

comment:14 followup: 17 Changed 5 years ago by
Branch:  → u/chapoton/13979 

Cc:  Vincent Delecroix Travis Scrimshaw added 
Commit:  → 88c38d9ebbaa39441795d648dcdef78c86c90c68 
Here is a minimal fix
but CartesianProduct? was deprecated in #18411 and should rather be fully removed
New commits:
88c38d9  trac 13979 changes in CartesianProduct

comment:15 Changed 5 years ago by
Status:  needs_work → needs_info 

comment:16 Changed 5 years ago by
Milestone:  sage6.4 → sage8.2 

comment:17 Changed 5 years ago by
comment:18 followup: 20 Changed 5 years ago by
Authors:  Christian Nassau → Christian Nassau, Frédéric Chapoton 

Status:  needs_info → needs_review 
So, is my branch an useful step after all ?
comment:20 Changed 5 years ago by
comment:21 Changed 5 years ago by
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 5 years ago by
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 5 years ago by
Commit:  88c38d9ebbaa39441795d648dcdef78c86c90c68 → 0c41d46d6c8dfc32ed1858db5ea38a3c2529f883 

Branch pushed to git repo; I updated commit sha1. New commits:
0c41d46  trac 13979 fixing doctests in dev_tools

comment:24 Changed 5 years ago by
Status:  needs_work → needs_review 

indeed, you are right. I have changed the failing doctest to use another class.
comment:25 Changed 5 years ago by
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 sideeffects (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 5 years ago by
Why not. But I have no idea of where to look for a better example...
comment:27 Changed 5 years ago by
Branch:  u/chapoton/13979 → u/tscrim/13979 

Commit:  0c41d46d6c8dfc32ed1858db5ea38a3c2529f883 → dd386711b472cb5ac2250c5bce8d1492aae6a8c3 
Reviewers:  → Travis Scrimshaw 
Sorry it took a little bit. Here is an example using sage.monoids
.
New commits:
dd38671  Changing test in dev_tools.py.

comment:30 Changed 5 years ago by
Branch:  u/tscrim/13979 → dd386711b472cb5ac2250c5bce8d1492aae6a8c3 

Resolution:  → fixed 
Status:  positive_review → closed 
Patch does not merge.