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: sage-8.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:

Status badges

Description (last modified by Ralf Stephan)

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 Christian Nassau 10 years ago.

Download all attachments as: .zip

Change History (31)

comment:1 Changed 10 years ago by Christian Nassau

Status: newneeds_review

comment:2 Changed 10 years ago by Christian Nassau

Authors: Christian Nassau
Description: modified (diff)

comment:3 Changed 10 years ago by Christian Nassau

Description: modified (diff)

comment:4 Changed 10 years ago by Christian Nassau

Description: modified (diff)
Summary: CartesianProduct.cardinality can't handle infinite setsMake CartesianProduct_iter a proper Parent

comment:5 Changed 10 years ago by Christian Nassau

Status: needs_reviewneeds_work

comment:6 Changed 10 years ago by Christian Nassau

Status: needs_workneeds_review

Changed 10 years ago by Christian Nassau

Attachment: cna13979.patch added

comment:7 Changed 9 years ago by Jeroen Demeyer

Milestone: sage-5.11sage-5.12

comment:8 Changed 9 years ago by For batch modifications

Milestone: sage-6.1sage-6.2

comment:9 Changed 9 years ago by For batch modifications

Milestone: sage-6.2sage-6.3

comment:10 Changed 9 years ago by Ralf Stephan

Status: needs_reviewneeds_work

Patch does not merge.

comment:11 Changed 9 years ago by Christian Nassau

Description: modified (diff)
Milestone: sage-6.3sage-duplicate/invalid/wontfix

comment:12 Changed 9 years ago by Ralf Stephan

Description: modified (diff)
Milestone: sage-duplicate/invalid/wontfixsage-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 8 years ago by For batch modifications

Milestone: sage-6.3sage-6.4

comment:14 Changed 5 years ago by Frédéric Chapoton

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:

88c38d9trac 13979 changes in CartesianProduct

comment:15 Changed 5 years ago by Frédéric Chapoton

Status: needs_workneeds_info

comment:16 Changed 5 years ago by Frédéric Chapoton

Milestone: sage-6.4sage-8.2

comment:17 in reply to:  14 Changed 5 years ago by Travis Scrimshaw

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 Changed 5 years ago by Frédéric Chapoton

Authors: Christian NassauChristian Nassau, Frédéric Chapoton
Status: needs_infoneeds_review

So, is my branch an useful step after all ?

comment:19 Changed 5 years ago by Frédéric Chapoton

Status: needs_reviewneeds_work

some failing doctests

comment:20 in reply to:  18 Changed 5 years ago by Travis Scrimshaw

Replying to chapoton:

So, is my branch an useful step after all ?

Yes, I think it is.

comment:21 Changed 5 years ago by Frédéric 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 5 years ago by Travis Scrimshaw

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 git

Commit: 88c38d9ebbaa39441795d648dcdef78c86c90c680c41d46d6c8dfc32ed1858db5ea38a3c2529f883

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

0c41d46trac 13979 fixing doctests in dev_tools

comment:24 Changed 5 years ago by Frédéric Chapoton

Status: needs_workneeds_review

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

comment:25 Changed 5 years ago by Travis Scrimshaw

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 5 years ago by Frédéric Chapoton

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

comment:27 Changed 5 years ago by Travis Scrimshaw

Branch: u/chapoton/13979u/tscrim/13979
Commit: 0c41d46d6c8dfc32ed1858db5ea38a3c2529f883dd386711b472cb5ac2250c5bce8d1492aae6a8c3
Reviewers: 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 5 years ago by Frédéric Chapoton

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

comment:29 Changed 5 years ago by Travis Scrimshaw

Status: needs_reviewpositive_review

Yep. Thanks.

comment:30 Changed 5 years ago by Volker Braun

Branch: u/tscrim/13979dd386711b472cb5ac2250c5bce8d1492aae6a8c3
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.