Opened 7 years ago
Closed 6 years ago
#18182 closed enhancement (fixed)
pushout construction and finding common parents for/including cartesian products
Reported by:  dkrenn  Owned by:  

Priority:  major  Milestone:  sage6.6 
Component:  coercion  Keywords:  sd67, coercion, categories 
Cc:  roed, behackl  Merged in:  
Authors:  Daniel Krenn, David Roe  Reviewers:  Benjamin Hackl, Daniel Krenn 
Report Upstream:  N/A  Work issues:  
Branch:  bc70cb9 (Commits, GitHub, GitLab)  Commit:  bc70cb9f592a7f3eb93267b91591d9cd4ae7b358 
Dependencies:  Stopgaps: 
Description
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.
Change History (25)
comment:1 Changed 7 years ago by
comment:2 Changed 7 years ago by
 Branch set to u/roed/18182
comment:3 Changed 6 years ago by
 Branch changed from u/roed/18182 to u/dkrenn/18182
comment:4 Changed 6 years ago by
 Commit set to 7f56908b5a4a12bb97e649cc971a55eec9152f5a
comment:5 followup: ↓ 13 Changed 6 years ago by
 Reviewers set to Daniel Krenn
 Status changed from new to needs_info
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?
comment:6 Changed 6 years ago by
 Commit changed from 7f56908b5a4a12bb97e649cc971a55eec9152f5a to f55cf6bde50cf7dd2e9841a3006646b7c331824a
Branch pushed to git repo; I updated commit sha1. New commits:
f55cf6b  Merge tag '6.8' into u/dkrenn/18182

comment:7 Changed 6 years ago by
 Branch changed from u/dkrenn/18182 to u/dkrenn/18182on6.7
 Commit changed from f55cf6bde50cf7dd2e9841a3006646b7c331824a to 7f56908b5a4a12bb97e649cc971a55eec9152f5a
comment:8 Changed 6 years ago by
 Work issues set to merge 6.8
comment:9 Changed 6 years ago by
 Commit changed from 7f56908b5a4a12bb97e649cc971a55eec9152f5a to cbb0d83f6daca32836b5b05fa9a2f055f05690b9
Branch pushed to git repo; I updated commit sha1. New commits:
cbb0d83  Merge tag '6.8' into u/dkrenn/18182

comment:10 Changed 6 years ago by
 Commit changed from cbb0d83f6daca32836b5b05fa9a2f055f05690b9 to 7f56908b5a4a12bb97e649cc971a55eec9152f5a
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
comment:11 Changed 6 years ago by
 Branch changed from u/dkrenn/18182on6.7 to u/dkrenn/18182on6.8
 Commit changed from 7f56908b5a4a12bb97e649cc971a55eec9152f5a to cbb0d83f6daca32836b5b05fa9a2f055f05690b9
 Work issues merge 6.8 deleted
New commits:
cbb0d83  Merge tag '6.8' into u/dkrenn/18182

comment:12 Changed 6 years ago by
 Commit changed from cbb0d83f6daca32836b5b05fa9a2f055f05690b9 to 2de49baf67030c02b926fb09e4470027f420bbd4
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 remotetracking branch 'origin/u/dkrenn/18182on6.7' into u/dkrenn/18182on6.8

23354db  add missing test and fix broken tests

2de49ba  Merge remotetracking branch 'origin/u/dkrenn/18182on6.7' into u/dkrenn/18182on6.8

comment:13 in reply to: ↑ 5 ; followup: ↓ 14 Changed 6 years ago by
 Status changed from needs_info to needs_work
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.
comment:14 in reply to: ↑ 13 Changed 6 years ago by
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 have an idea? (I have no glue)
comment:15 Changed 6 years ago by
 Commit changed from 2de49baf67030c02b926fb09e4470027f420bbd4 to 60b9375bd3733f5b0a1a802504b8348a8feb6795
comment:16 Changed 6 years ago by
 Branch changed from u/dkrenn/18182on6.8 to u/dkrenn/18182/pushout
 Status changed from needs_work to needs_review
reverted the change in base_ring of category_object (this will be a separate ticket (#19225))
comment:17 Changed 6 years ago by
 Status changed from needs_review to needs_work
comment:18 Changed 6 years ago by
 Commit changed from 60b9375bd3733f5b0a1a802504b8348a8feb6795 to c16587c8b549dde5891f00ec9be10426889c7424
Branch pushed to git repo; I updated commit sha1. New commits:
c16587c  fix bug (tower has only one entry which is None)

comment:19 Changed 6 years ago by
 Status changed from needs_work to needs_review
Bug fixed...let's see what the patchbot does...
comment:20 followup: ↓ 21 Changed 6 years ago by
 Branch changed from u/dkrenn/18182/pushout to u/behackl/coercion/pushout
 Commit changed from c16587c8b549dde5891f00ec9be10426889c7424 to d449fabb1adcd514bf988e64736a18de34dd7e25
 Reviewers changed from Daniel Krenn to Benjamin Hackl, Daniel Krenn
 Status changed from needs_review to needs_work
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 followup 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 thatRaise a CoercionException
does not fit in aOUTPUT
block from a semantic point of view.
categories.pushout.MultivariateConstructionFunctor
: Is there a reason for the import ofCartesianProduct
in theTESTS
block? (I've removed the import in one of my commits.)
MultivariateConstructionFunctor.common_base
: Could you explain why you useget_coercion_model().common_parent(...)
instead ofpushout(...)
?
pushout
: Is there a reason for usingsage: from sage.sets.cartesian_product import CartesianProduct sage: A = CartesianProduct((ZZ['x'], QQ['y'], QQ['z']), Sets().CartesianProducts())
oversage: 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 examplesage: 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 aCoercionException
is thrown (instead of anAttributeError
for the failed call toR_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. #18223;)
).
Benjamin
New commits:
5c29fd4  fix typos

6233426  improve language

d31667e  fix ReSterror

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

comment:21 in reply to: ↑ 20 ; followup: ↓ 24 Changed 6 years ago by
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 followup 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 thatRaise a CoercionException
does not fit in aOUTPUT
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 ofCartesianProduct
in theTESTS
block? (I've removed the import in one of my commits.)
Thanks.
MultivariateConstructionFunctor.common_base
: Could you explain why you useget_coercion_model().common_parent(...)
instead ofpushout(...)
?
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 usingsage: from sage.sets.cartesian_product import CartesianProduct sage: A = CartesianProduct((ZZ['x'], QQ['y'], QQ['z']), Sets().CartesianProducts())oversage: 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 examplesage: from sage.categories.pushout import pushout sage: pushout(ZZ, cartesian_product([ZZ, QQ])) Traceback (most recent call last): ... CoercionException: 'NoneType' object is not iterablethat aCoercionException
is thrown (instead of anAttributeError
for the failed call toR_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. #18223;)
).
Done.
Crossreviewed your changes...seem to be fine.
comment:22 Changed 6 years ago by
 Branch changed from u/behackl/coercion/pushout to u/dkrenn/coercion/pushout
comment:23 Changed 6 years ago by
 Commit changed from d449fabb1adcd514bf988e64736a18de34dd7e25 to bc70cb9f592a7f3eb93267b91591d9cd4ae7b358
 Status changed from needs_work to needs_review
comment:24 in reply to: ↑ 21 Changed 6 years ago by
 Status changed from needs_review to positive_review
Replying to dkrenn:
categories.modules.CartesianProducts
: I'm not entirely sure [...]This is now #19375.
Perfect.
categories.pushout.ConstructionFunctor.common_base
: I think thatRaise a CoercionException
does not fit in aOUTPUT
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 useget_coercion_model().common_parent(...)
instead ofpushout(...)
?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 usingsage: from sage.sets.cartesian_product import CartesianProduct sage: A = CartesianProduct((ZZ['x'], QQ['y'], QQ['z']), Sets().CartesianProducts())oversage: 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 crosschecked your changes, everything seems to work now, and I have no more comments.
The documentation builds and make ptestlong
passes. > positive_review
.
comment:25 Changed 6 years ago by
 Branch changed from u/dkrenn/coercion/pushout to bc70cb9f592a7f3eb93267b91591d9cd4ae7b358
 Resolution set to fixed
 Status changed from positive_review to closed
(see also #15425)