Opened 9 years ago
Closed 4 years ago
#13979 closed defect (fixed)
Make CartesianProduct_iter a proper Parent
Reported by:  cnassau  Owned by:  sagecombinat 

Priority:  major  Milestone:  sage8.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: 
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 9 years ago by
 Status changed from new to needs_review
comment:2 Changed 9 years ago by
 Description modified (diff)
comment:3 Changed 9 years ago by
 Description modified (diff)
comment:4 Changed 9 years ago by
 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
 Status changed from needs_review to needs_work
comment:6 Changed 9 years ago by
 Status changed from needs_work to needs_review
Changed 9 years ago by
comment:7 Changed 8 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:8 Changed 8 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:9 Changed 8 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:10 Changed 8 years ago by
 Status changed from needs_review to needs_work
comment:11 Changed 8 years ago by
 Description modified (diff)
 Milestone changed from sage6.3 to sageduplicate/invalid/wontfix
comment:12 Changed 8 years ago by
 Description modified (diff)
 Milestone changed from sageduplicate/invalid/wontfix to 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 7 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:14 followup: ↓ 17 Changed 4 years ago by
 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:
88c38d9  trac 13979 changes in CartesianProduct

comment:15 Changed 4 years ago by
 Status changed from needs_work to needs_info
comment:16 Changed 4 years ago by
 Milestone changed from sage6.4 to sage8.2
comment:17 in reply to: ↑ 14 Changed 4 years ago by
comment:18 followup: ↓ 20 Changed 4 years ago by
 Status changed from needs_info to needs_review
So, is my branch an useful step after all ?
comment:19 Changed 4 years ago by
 Status changed from needs_review to needs_work
some failing doctests
comment:20 in reply to: ↑ 18 Changed 4 years ago by
comment:21 Changed 4 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 4 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 4 years ago by
 Commit changed from 88c38d9ebbaa39441795d648dcdef78c86c90c68 to 0c41d46d6c8dfc32ed1858db5ea38a3c2529f883
Branch pushed to git repo; I updated commit sha1. New commits:
0c41d46  trac 13979 fixing doctests in dev_tools

comment:24 Changed 4 years ago by
 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
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 4 years ago by
Why not. But I have no idea of where to look for a better example...
comment:27 Changed 4 years ago by
 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:
dd38671  Changing test in dev_tools.py.

comment:28 Changed 4 years ago by
ok, good to go for me. Travis, please confirm.
comment:30 Changed 4 years ago by
 Branch changed from u/tscrim/13979 to dd386711b472cb5ac2250c5bce8d1492aae6a8c3
 Resolution set to fixed
 Status changed from positive_review to closed
Patch does not merge.