Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Meta-ticket: Cleanup cartesian products #15425

Open
nthiery opened this issue Nov 15, 2013 · 19 comments
Open

Meta-ticket: Cleanup cartesian products #15425

nthiery opened this issue Nov 15, 2013 · 19 comments

Comments

@nthiery
Copy link
Contributor

nthiery commented Nov 15, 2013

Currently we have: cartesian_product, CartesianProduct and
cartesian_product_iterator for constructing cartesian products.

  • CartesianProduct is an old simple parent that focuses on the
    "enumerated sets" aspect: providing counting and enumeration over
    cartesian products of enumerated sets. It accepts any iterables as
    input.

  • cartesian_product is a "functorial construction". This means
    that it uses the categories to endow the resulting parent with as
    much structure as it can lift from the input. E.g. the cartesian
    product of two monoids is a monoid.

  • cartesian_product_iterator is just a function that provides an
    iterator

To be done:

  1. get rid of CartesianProduct #18411: Make CartesianProduct an alias for cartesian_product, and possibly deprecated it. The missing features at this point are:

  2. Deprecate sage.misc.mrange.*mrange* and cartesian_product_iterator #34337: Remove cartesian_product_iterator from the global name space, and deprecate it altogether if, after checking, it turns out to be really just a duplicated of itertools.product.

  3. Fix ZZ.cartesian_product(...) #16289: Fix bug in cartesian_product (reported by Vincent Delecroix in this thread):

    sage: C = cartesian_product([ZZ,ZZ])
    ...
    AttributeError: type object 'sage.rings.integer_ring.IntegerRing_class' has no attribute 'CartesianProduct'
    

    This is a regression that is caused by a small change introduced by
    Improvements to Sets.WithRealizations #12959 in Sets.ParentMethods.CartesianProduct:

        return parents[0].__class__ -> return parents[0].__class__
    

    I (Nicolas) take a double blame for it: I was reviewer of this ticket
    and did not notice this chunk (or don't remember why it was
    introduced), and I had not written an appropriate test in the first
    place. So this needs to be fixed too.

  4. Cartesian Products of additive groups #16269: Fix bug reported by Nathann Cohen in this thread: when converting a
    list to an element of a cartesian product:

    sage: Z3 = IntegerModRing(3)
    sage: C = cartesian_product([Z3,Z3])
    sage: C([Z3(2),Z3(2)])^2
    (1, 1)
    sage: C([2,2])^2   # Ooops
    (4, 4)
    

    The fix would be convert the operands of the list into the respective
    parents in
    sage.sets.cartesian_product.CartesianProduct._element_constructor.

  5. Fix mixed cartesian products with modules and non modules:

    sage: A = AlgebrasWithBasis(QQ).example(); A.rename("A")
    sage: cartesian_product([A, ZZ])
    ...
    AttributeError: 'sage.rings.integer_ring.IntegerRing_class' object has no attribute 'basis'
    

    This should instead detect that not all factors are modules, and
    just use a plain cartesian product.

    Also between modules on different ring, in particular Categories are wrong for Hom/cartesian products of vector spaces #18309.

  6. Import NN directly rather than lazily throughout the Sage library #34652: Fix cartesian products involving NN:

    sage: cartesian_product([NN,NN])
    170         from sage.structure.parent import Parent
    --> 171         assert(all(isinstance(parent, Parent) for parent in parents))
    172         # Should we pass a set of categories to reduce the cache size?
    173         # But then this would impose that, for any constructor, the
    AssertionError: 
    

    This is in fact a bug in the way NN is lazy imported in the global
    name space:

        sage: type(NN)
    <type 'sage.misc.lazy_import.LazyImport'>
    sage: isinstance(NN, Parent)
        False
    

    Things works if one forces the import of NN:

    sage: NN = NonNegativeIntegers()
    sage: cartesian_product([NN,NN])
    The cartesian product of (Non negative integers, Non negative integers)
    
  7. Make _cartesian_product_of_elements a public method?

  8. Add a tutorial in Sets.SubcategoryMethods.CartesianProducts
    describing the general scheme, possibly starting from the blurb there:
    https://groups.google.com/d/msg/sage-combinat-devel/s_aPBD6BgOg/H1aJbCI1TYoJ

  9. Tidy up the documentation of sage.sets.cartesian_products:
    Return(s), the links to Sets.... don't need to be prefixed with
    the python module (Sets is found from the global name space), ...

  10. Cartesian Products of additive groups #16269 and follow up Cartesian product of rings #16405 (depended on Axioms and more functorial constructions #10963): make the
    cartesian product of an additive magma into an additive magma, and
    so on; implement Distributive.CartesianProducts so that a
    cartesian product of rings is a ring.

CC: @sagetrac-sage-combinat @nathanncohen @videlec @tscrim

Component: categories

Issue created by migration from https://trac.sagemath.org/ticket/15425

@nthiery nthiery added this to the sage-6.1 milestone Nov 15, 2013
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.1, sage-6.2 Jan 30, 2014
@nthiery

This comment has been minimized.

@nthiery

This comment has been minimized.

@nthiery nthiery removed the t: bug label May 5, 2014
@videlec

This comment has been minimized.

@videlec
Copy link
Contributor

videlec commented May 5, 2014

comment:4

Hello,

+1 on that ticket!

I cleaned a bit the description.

I think it is bad to hide cartesian_product_iterator as the object is very nice. We should advertise the product from itertools somewhere.

Vincent

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented May 5, 2014

comment:5

I think it is bad to hide cartesian_product_iterator as the object is very nice. We should advertise the product from itertools somewhere.

We could mention it in the docstring of cartesian_product. "If you just want to list the elements, use itertools.product as it is much faster" ?

@nthiery
Copy link
Contributor Author

nthiery commented May 5, 2014

comment:6

Replying to @nathanncohen:

I think it is bad to hide cartesian_product_iterator as the object is very nice. We should advertise the product from itertools somewhere.

We could mention it in the docstring of cartesian_product. "If you just want to list the elements, use itertools.product as it is much faster" ?

Sounds good to me, with 'list -> iterate through'

@nthiery

This comment has been minimized.

@nthiery

This comment has been minimized.

@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.2, sage-6.3 May 6, 2014
@nthiery

This comment has been minimized.

@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.3, sage-6.4 Aug 10, 2014
@videlec

This comment has been minimized.

@videlec
Copy link
Contributor

videlec commented Apr 23, 2015

comment:13

Hello,

I think that we should get rid of _cartesian_product_of_elements. If we want speed we can either:

  • add an argument coerce to the _element_constructor_
  • use directly element_class

Vincent

@videlec
Copy link
Contributor

videlec commented Apr 23, 2015

comment:14

Shouldn't the two following commands give the same answer

sage: ZZ**2
Ambient free module of rank 2 over the principal ideal domain Integer Ring
sage: cartesian_product([ZZ,ZZ])
The cartesian product of (Integer Ring, Integer Ring)

@videlec

This comment has been minimized.

@fchapoton

This comment has been minimized.

@videlec

This comment has been minimized.

@videlec videlec modified the milestones: sage-6.4, sage-6.9 Sep 13, 2015
@mkoeppe mkoeppe modified the milestones: sage-6.9, sage-9.7 Aug 10, 2022
@mkoeppe mkoeppe changed the title Cleanup cartesian products Meta-ticket: Cleanup cartesian products Aug 10, 2022
@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 11, 2022

comment:20

Bugs 5 and 6 are still present in 9.7.beta8

@mkoeppe mkoeppe modified the milestones: sage-9.7, sage-9.8 Aug 31, 2022
@mkoeppe
Copy link
Contributor

mkoeppe commented Jan 2, 2023

comment:22

Bug 6 was fixed in #34652

@mkoeppe

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants