Opened 6 years ago

Closed 6 years ago

#16289 closed defect (fixed)

Fix ZZ.cartesian_product(...)

Reported by: nthiery Owned by:
Priority: major Milestone: sage-6.3
Component: combinatorics Keywords:
Cc: sage-combinat, ncohen, vdelecroix Merged in:
Authors: Nicolas M. Thiéry Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: c9a7e67 (Commits) Commit: c9a7e6728d1689b828edb5b367d69a89a9a0cf66
Dependencies: #16280, #16288, #16269 Stopgaps:

Description

This fails due to an inadvertent change introduced by #12959:

sage: ZZ.cartesian_product(GF(2), NN)
Traceback (most recent call last)
...
AttributeError: type object 'sage.rings.integer_ring.IntegerRing_class' has no attribute 'CartesianProduct'

This ticket reverts this change and adds a doctest.

Change History (15)

comment:1 Changed 6 years ago by nthiery

  • Branch set to u/nthiery/16269-optimized

comment:2 Changed 6 years ago by nthiery

  • Branch changed from u/nthiery/16269-optimized to u/nthiery/16289/fix_zz_cartesian_product

comment:3 Changed 6 years ago by nthiery

  • Commit set to c9a7e6728d1689b828edb5b367d69a89a9a0cf66
  • Status changed from new to needs_review

Last 10 new commits:

aa07ba1trac #16269: Working around "officially not a bug"
d22e245trac #16269: missing __iter__
f262faatrac #16269: Merged with 6.2.rc1
338cc5btrac #16269: Reviewer's remarks
8ac32c2#16280: Fix call for FiniteEnumeratedSet's of plain Python objects
4e27454#16280: Trivial doctest fixes
f67d82etrac #16269: Merged with #16280
0a54b68Trac 16269: little improvements + line folding in the doctest output
b5ad803Trac 16269 (or follow up): optimize CartesianProduct._cartesian_product_of_elements
c9a7e67#16289: Fix ZZ.cartesian_product(...)

comment:4 Changed 6 years ago by tscrim

  • Dependencies set to #16280

comment:5 Changed 6 years ago by ncohen

This branch apparently includes #16269 as a dependency... But perhaps that is not a problem anyway as it is already reviewed ?

Nathann

comment:6 Changed 6 years ago by ncohen

........................... and this branch also depends on the commit labeled with "rac 16269 (or follow up)" which is actually not #16269 but #16288...

Nathann

comment:7 follow-up: Changed 6 years ago by ncohen

  • Dependencies changed from #16280 to #16280, #16288, #16269

(I set the dependencies that appear in the branch, please update it if you rewrite the branch as some of these dependencies are not necessary. But of course possibly we don't care for as long as everything gets reviewed)

comment:8 in reply to: ↑ 7 Changed 6 years ago by nthiery

Replying to ncohen:

(I set the dependencies that appear in the branch, please update it if you rewrite the branch as some of these dependencies are not necessary. But of course possibly we don't care for as long as everything gets reviewed)

There actually is a small semantic dependency upon #16269 since it changes the output of one of the doctests of this ticket. For #16288, oh well. Let's just keep the branch as it is. As you say we don't really care since everything is positive reviewed or close enough to be so.

comment:9 follow-up: Changed 6 years ago by ncohen

Yoooooo !

Looks totally nice... Do you have any idea why there was a __class__ there ? It does not make any sense to me O_o

Nathann

comment:10 in reply to: ↑ 9 Changed 6 years ago by nthiery

Replying to ncohen:

Looks totally nice... Do you have any idea why there was a __class__ there ? It does not make any sense to me O_o

Go figure ... This looks a bit more like a call to a class method rather than a call to a usual method, since we pass explicitly self as first argument. So maybe the point was to emphasize that? Anyway we don't need it, so it's as good without it.

comment:11 Changed 6 years ago by ncohen

Yep it seems clearer this way.... O_O

Okay, good to go... I need to calm down a bit. I just "reviewed" #16246.

Did you check the doctests ?

Nathann

comment:12 Changed 6 years ago by nthiery

Yup, all long tests passed for me!

comment:13 Changed 6 years ago by ncohen

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

Okay, so positive review and I will check the docstest again on my office's computer again just in case, now that Leandro turned it on again.

Nathann

comment:14 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:15 Changed 6 years ago by vbraun

  • Branch changed from u/nthiery/16289/fix_zz_cartesian_product to c9a7e6728d1689b828edb5b367d69a89a9a0cf66
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.