pushout construction and finding common parents for/including cartesian products

[dkrenn]

sage: cartesian_product([ZZ]).construction() is None
True

and the coercion model (in partiuclar, the pushout function) does not take care of cartesian products. The aim of this ticket is to add functionality for doing so.

[ncohen]

(see also #15425)

[git]

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

​fe83e88	update authors of pushout
​7f56908	fix links in docu

[dkrenn]

I've reviewed David's part, but I am also an author of this ticket; thus I request an additional reviewer.

Note that there is still one failing doctest, where I don't have any glue why (it is the bug at the very top of the file). Who knows what is going on there?

[git]

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

​f55cf6b

Merge tag '6.8' into u/dkrenn/18182

[git]

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

​cbb0d83

Merge tag '6.8' into u/dkrenn/18182

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

[dkrenn]

New commits:

​cbb0d83

Merge tag '6.8' into u/dkrenn/18182

[git]

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

​3cacc52	change recursive pushout to common_parent (to take care of direct coercions)
​4662eed	bug kind of solved (called now directly by cartesian_product); move doctest
​ce20dd9	Merge remote-tracking branch 'origin/u/dkrenn/18182-on-6.7' into u/dkrenn/18182-on-6.8
​23354db	add missing test and fix broken tests
​2de49ba	Merge remote-tracking branch 'origin/u/dkrenn/18182-on-6.7' into u/dkrenn/18182-on-6.8

[dkrenn]

Replying to dkrenn:

Note that there is still one failing doctest, where I don't have any glue why (it is the bug at the very top of the file). Who knows what is going on there?

Kind of fixed; now calling the cartesian product by cartesian_product, which produces the correct result.

There are still two failing doctests.

[dkrenn]

Replying to dkrenn:

There are still two failing doctests.

File "src/sage/categories/homset.py", line 596, in sage.categories.homset.Homset.__init__
Failed example:
    MyHomset(ZZ^3, ZZ^3, base = QQ).base_ring()
Expected:
    Rational Field
Got:
    Integer Ring

File "src/sage/categories/modules.py", line 519, in sage.categories.modules.Modules.Homsets.ParentMethods.base_ring
Failed example:
    H.base_ring.__module__
Expected nothing
Got:
    'sage.categories.modules'

Does someone has an idea?

[git]

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

​cd17673	Merge tag '6.9.beta6' into t/18182/18182-on-6.8
​3eefe25	correct typo in AUTHORS
​5fe52e4	fix doctests since name of cartesian product functor has changed
​60b9375	revert changes in base_ring of category_object and adapt doctests

[dkrenn]

reverted the change in base_ring of category_object (this will be a separate ticket (#19225))

[git]

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

​c16587c

fix bug (tower has only one entry which is None)

[dkrenn]

Bug fixed...let's see what the patchbot does...

[behackl]

Hello!

I had a look at the changes on this ticket, made a few reviewer commits, and I have the following comments:

1. categories.modules.CartesianProducts: I'm not entirely sure about how everything is handled here:
   • ParentMethods.base_ring: It seems as if the base ring of the cartesian product of some modules is considered to be the base ring of the first module in the list. Is this intended?
   • Should different base rings of the modules in a cartesian product be allowed? If so, this has to be fixed:

       sage: E = CombinatorialFreeModule(ZZ, [1,2])
       sage: F = CombinatorialFreeModule(QQ, ['a', 'b'])
       sage: cartesian_product([E, F])
       Traceback (most recent call last):
       ...
       AttributeError: 'CommutativeAdditiveGroups.CartesianProducts_with_c' object has no attribute 'FiniteDimensional'
     

   • Otherwise, if this should not be supported, the error message should be replaced by something more meaningful.

Note that fixing these things would be suitable for a follow-up ticket, as the current behavior is (as far as I can tell) correct: if the base rings of the modules coincide, the cartesian product can be built (at least in the examples I tried). Otherwise, a (unfortunately rather strange) exception is raised. And in the case where all base rings coincide, base_ring can of course return the base ring of any of the passed modules.

2. categories.pushout.ConstructionFunctor.common_base: I think that Raise a CoercionException does not fit in a OUTPUT-block from a semantic point of view.

3. categories.pushout.MultivariateConstructionFunctor: Is there a reason for the import of CartesianProduct in the TESTS-block? (I've removed the import in one of my commits.)

4. MultivariateConstructionFunctor.common_base: Could you explain why you use get_coercion_model().common_parent(...) instead of pushout(...)?

5. pushout: Is there a reason for using

     sage: from sage.sets.cartesian_product import CartesianProduct
     sage: A = CartesianProduct((ZZ['x'], QQ['y'], QQ['z']), Sets().CartesianProducts())
   

   over

     sage: A = cartesian_product((ZZ['x'], QQ['y'], QQ['z']))
   

   As far as I can tell, the only difference is in the categories -- but they aren't used in these doctests.

6. pushout: Am I right in the assumption that the reason for the check .. and R_tower[-1][0] is not None is that when considering, for example

     sage: from sage.categories.pushout import pushout
     sage: pushout(ZZ, cartesian_product([ZZ, QQ]))
     Traceback (most recent call last):
     ...
     CoercionException: 'NoneType' object is not iterable
   

   that a CoercionException is thrown (instead of an AttributeError for the failed call to R_tower[-1][0].common_base(...) in the line after the check)? Or is there more to it? Would it make sense to add a doctest for this exact case? (I've included one in my reviewer commits; proceed with it as you like.)

7. pushout: CartesianProductPolys vs. CartesianProductPoly? (cf. #18223 ;-)).

Benjamin

---

New commits:

​5c29fd4	fix typos
​6233426	improve language
​d31667e	fix ReSt-error
​c4a5bfd	remove superfluous import in doctest, superfluous empty line in docstring, and fix spacing in a line (pep 8)
​726b74a	CartesianProductPolys: check whether other has a construction
​d449fab	add two doctests

[dkrenn]

Replying to behackl:

1. categories.modules.CartesianProducts: I'm not entirely sure about how everything is handled here:
   • ParentMethods.base_ring: It seems as if the base ring of the cartesian product of some modules is considered to be the base ring of the first module in the list. Is this intended?
   • Should different base rings of the modules in a cartesian product be allowed? If so, this has to be fixed:

       sage: E = CombinatorialFreeModule(ZZ, [1,2])
       sage: F = CombinatorialFreeModule(QQ, ['a', 'b'])
       sage: cartesian_product([E, F])
       Traceback (most recent call last):
       ...
       AttributeError: 'CommutativeAdditiveGroups.CartesianProducts_with_c' object has no attribute 'FiniteDimensional'
     

   • Otherwise, if this should not be supported, the error message should be replaced by something more meaningful.

Note that fixing these things would be suitable for a follow-up ticket, as the current behavior is (as far as I can tell) correct: if the base rings of the modules coincide, the cartesian product can be built (at least in the examples I tried). Otherwise, a (unfortunately rather strange) exception is raised. And in the case where all base rings coincide, base_ring can of course return the base ring of any of the passed modules.

This is now #19375.

2. categories.pushout.ConstructionFunctor.common_base: I think that Raise a CoercionException does not fit in a OUTPUT-block from a semantic point of view.

Rewritten (but I am open to suggestions if you want something different)

3. categories.pushout.MultivariateConstructionFunctor: Is there a reason for the import of CartesianProduct in the TESTS-block? (I've removed the import in one of my commits.)

Thanks.

4. MultivariateConstructionFunctor.common_base: Could you explain why you use get_coercion_model().common_parent(...) instead of pushout(...)?

We need a common parent, thus common_parent is the correct method to call. A pushout is one possible way to construct a common parent, but there are other ways; e.g. one could coerce into the other, or there is a scalar multiplication or action available.

5. pushout: Is there a reason for using

     sage: from sage.sets.cartesian_product import CartesianProduct
     sage: A = CartesianProduct((ZZ['x'], QQ['y'], QQ['z']), Sets().CartesianProducts())
   

   over

     sage: A = cartesian_product((ZZ['x'], QQ['y'], QQ['z']))
   

   As far as I can tell, the only difference is in the categories -- but they aren't used in these doctests.

This is to check that it works as well (there was once a bug with this kind of construction, thus a doctest was added).

6. pushout: Am I right in the assumption that the reason for the check .. and R_tower[-1][0] is not None is that when considering, for example

     sage: from sage.categories.pushout import pushout
     sage: pushout(ZZ, cartesian_product([ZZ, QQ]))
     Traceback (most recent call last):
     ...
     CoercionException: 'NoneType' object is not iterable
   

   that a CoercionException is thrown (instead of an AttributeError for the failed call to R_tower[-1][0].common_base(...) in the line after the check)?

Yes, this is the reason. An AttributeError does not work with some of the calling functions; they need a CoercionException?`.

Or is there more to it?

No.

Would it make sense to add a doctest for this exact case? (I've included one in my reviewer commits; proceed with it as you like.)

Thanks.

7. pushout: CartesianProductPolys vs. CartesianProductPoly? (cf. #18223 ;-)).

Done.

Cross-reviewed your changes...seem to be fine.

[dkrenn]

New commits:

​4619fe7	Trac #18182, comment 20, 2: rewrite OUTPUT-block
​bc70cb9	Trac #18182, comment 20, 7: rename CartesianProductPolys --> CartesianProductPoly

[behackl]

Replying to dkrenn:

1. categories.modules.CartesianProducts: I'm not entirely sure [...]

This is now #19375.

Perfect.

2. categories.pushout.ConstructionFunctor.common_base: I think that Raise a CoercionException does not fit in a OUTPUT-block from a semantic point of view.

Rewritten (but I am open to suggestions if you want something different)

No, the way you've rewritten it is fine.

4. MultivariateConstructionFunctor.common_base: Could you explain why you use get_coercion_model().common_parent(...) instead of pushout(...)?

We need a common parent, thus common_parent is the correct method to call. A pushout is one possible way to construct a common parent, but there are other ways; e.g. one could coerce into the other, or there is a scalar multiplication or action available.

I understand.

5. pushout: Is there a reason for using

     sage: from sage.sets.cartesian_product import CartesianProduct
     sage: A = CartesianProduct((ZZ['x'], QQ['y'], QQ['z']), Sets().CartesianProducts())
   

   over

     sage: A = cartesian_product((ZZ['x'], QQ['y'], QQ['z']))
   

   As far as I can tell, the only difference is in the categories -- but they aren't used in these doctests.

This is to check that it works as well (there was once a bug with this kind of construction, thus a doctest was added).

Alright.

I cross-checked your changes, everything seems to work now, and I have no more comments.

The documentation builds and make ptestlong passes. --> positive_review.