pushout construction and finding common parents for/including cartesian products #18182

dkrenn · 2015-04-13T21:34:15Z

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.

CC: @roed314 @behackl

Component: coercion

Keywords: sd67

Author: Daniel Krenn, David Roe

Branch/Commit: bc70cb9

Reviewer: Benjamin Hackl, Daniel Krenn

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

The text was updated successfully, but these errors were encountered:

nathanncohen · 2015-04-14T08:21:16Z

comment:1

(see also #15425)

roed314 · 2015-04-14T21:36:24Z

Branch: u/roed/18182

dkrenn · 2015-08-20T12:21:32Z

Changed branch from u/roed/18182 to u/dkrenn/18182

sagetrac-git · 2015-08-20T12:36:15Z

Commit: 7f56908

sagetrac-git · 2015-08-20T12:36:15Z

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

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

dkrenn · 2015-08-20T12:40:25Z

Author: Daniel Krenn, David Roe

dkrenn · 2015-08-20T12:40:25Z

Reviewer: Daniel Krenn

dkrenn · 2015-08-20T12:40:25Z

comment:5

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?

sagetrac-git · 2015-08-20T12:44:32Z

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

`f55cf6b`	`Merge tag '6.8' into u/dkrenn/18182`

sagetrac-git · 2015-08-20T12:44:32Z

Changed commit from 7f56908 to f55cf6b

dkrenn · 2015-08-20T12:47:38Z

Changed branch from u/dkrenn/18182 to u/dkrenn/18182-on-6.7

dkrenn · 2015-08-20T12:47:38Z

Changed commit from f55cf6b to 7f56908

dkrenn · 2015-08-20T12:47:53Z

Work Issues: merge 6.8

sagetrac-git · 2015-08-20T12:52:08Z

Changed commit from 7f56908 to cbb0d83

sagetrac-git · 2015-08-20T12:52:08Z

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

`cbb0d83`	`Merge tag '6.8' into u/dkrenn/18182`

sagetrac-git · 2015-08-20T12:52:59Z

Changed commit from cbb0d83 to 7f56908

sagetrac-git · 2015-08-20T12:52:59Z

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

dkrenn · 2015-08-20T12:54:06Z

New commits:

`cbb0d83`	`Merge tag '6.8' into u/dkrenn/18182`

dkrenn · 2015-08-20T12:54:06Z

Changed branch from u/dkrenn/18182-on-6.7 to u/dkrenn/18182-on-6.8

dkrenn · 2015-08-20T12:54:06Z

Changed commit from 7f56908 to cbb0d83

dkrenn · 2015-08-20T12:54:06Z

Changed work issues from merge 6.8 to none

sagetrac-git · 2015-08-23T09:01:06Z

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`

sagetrac-git · 2015-08-23T09:01:06Z

Changed commit from cbb0d83 to 2de49ba

sagetrac-git · 2015-09-16T17:34:18Z

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

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

sagetrac-git · 2015-09-16T17:34:18Z

Changed commit from 60b9375 to c16587c

dkrenn · 2015-09-16T17:35:09Z

comment:19

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

behackl · 2015-10-04T23:08:51Z

comment:20

Hello!

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

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.

categories.pushout.ConstructionFunctor.common_base: I think that Raise a CoercionException does not fit in a OUTPUT-block from a semantic point of view.
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.)
MultivariateConstructionFunctor.common_base: Could you explain why you use get_coercion_model().common_parent(...) instead of pushout(...)?
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.

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.)

pushout: CartesianProductPolys vs. CartesianProductPoly? (cf. cartesian products with orders #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`

behackl · 2015-10-04T23:08:51Z

Changed reviewer from Daniel Krenn to Benjamin Hackl, Daniel Krenn

behackl · 2015-10-04T23:08:51Z

Changed commit from c16587c to d449fab

behackl · 2015-10-04T23:08:51Z

Changed branch from u/dkrenn/18182/pushout to u/behackl/coercion/pushout

dkrenn · 2015-10-08T11:44:01Z

comment:21

Replying to @behackl:

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.

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)

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.

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.

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).

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.

pushout: CartesianProductPolys vs. CartesianProductPoly? (cf. cartesian products with orders #18223 ;-)).

Done.

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

dkrenn · 2015-10-08T11:44:16Z

Changed branch from u/behackl/coercion/pushout to u/dkrenn/coercion/pushout

dkrenn · 2015-10-08T11:44:37Z

New commits:

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

dkrenn · 2015-10-08T11:44:37Z

Changed commit from d449fab to bc70cb9

behackl · 2015-10-13T16:42:50Z

comment:24

Replying to @dkrenn:

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

This is now #19375.

Perfect.

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.

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.

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.

vbraun · 2015-10-14T17:03:39Z

Changed branch from u/dkrenn/coercion/pushout to bc70cb9

dkrenn added this to the sage-6.6 milestone Apr 13, 2015

dkrenn added c: coercion labels Apr 13, 2015

dkrenn added the s: needs info label Aug 20, 2015

dkrenn added s: needs review and removed s: needs work labels Sep 16, 2015

behackl added s: needs work and removed s: needs review labels Oct 4, 2015

dkrenn added s: needs review and removed s: needs work labels Oct 8, 2015

behackl added s: positive review and removed s: needs review labels Oct 13, 2015

vbraun removed the s: positive review label Oct 14, 2015

vbraun closed this as completed in dca6c0d Oct 14, 2015

behackl mentioned this issue Dec 29, 2022

Meta ticket: Asymptotic Expansions in SageMath #17601

Open

This was referenced Oct 24, 2015

categorial constructions, pushout and coercions (extended) for asymptotic ring and growth groups #19073

Closed

cartesian product of modules over different base rings #19375

Open

pushout construction and finding common parents for/including cartesian products #18182

pushout construction and finding common parents for/including cartesian products #18182

Comments

dkrenn commented Apr 13, 2015

nathanncohen mannequin commented Apr 14, 2015

roed314 commented Apr 14, 2015

dkrenn commented Aug 20, 2015

sagetrac-git mannequin commented Aug 20, 2015

sagetrac-git mannequin commented Aug 20, 2015

dkrenn commented Aug 20, 2015

dkrenn commented Aug 20, 2015

dkrenn commented Aug 20, 2015

sagetrac-git mannequin commented Aug 20, 2015

sagetrac-git mannequin commented Aug 20, 2015

dkrenn commented Aug 20, 2015

dkrenn commented Aug 20, 2015

dkrenn commented Aug 20, 2015

sagetrac-git mannequin commented Aug 20, 2015

sagetrac-git mannequin commented Aug 20, 2015

sagetrac-git mannequin commented Aug 20, 2015

sagetrac-git mannequin commented Aug 20, 2015

dkrenn commented Aug 20, 2015

dkrenn commented Aug 20, 2015

dkrenn commented Aug 20, 2015

dkrenn commented Aug 20, 2015

sagetrac-git mannequin commented Aug 23, 2015

sagetrac-git mannequin commented Aug 23, 2015

sagetrac-git mannequin commented Sep 16, 2015

sagetrac-git mannequin commented Sep 16, 2015

dkrenn commented Sep 16, 2015

behackl commented Oct 4, 2015

behackl commented Oct 4, 2015

behackl commented Oct 4, 2015

behackl commented Oct 4, 2015

dkrenn commented Oct 8, 2015

dkrenn commented Oct 8, 2015

dkrenn commented Oct 8, 2015

dkrenn commented Oct 8, 2015

behackl commented Oct 13, 2015

vbraun commented Oct 14, 2015