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

Priority:  major  Milestone:  sage6.6 
Component:  coercion  Keywords:  sd67, coercion, categories 
Cc:  David Roe, Benjamin Hackl  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:  → u/roed/18182 

comment:3 Changed 7 years ago by
Branch:  u/roed/18182 → u/dkrenn/18182 

comment:4 Changed 7 years ago by
Commit:  → 7f56908b5a4a12bb97e649cc971a55eec9152f5a 

comment:5 followup: 13 Changed 7 years ago by
Authors:  → Daniel Krenn, David Roe 

Reviewers:  → Daniel Krenn 
Status:  new → 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 7 years ago by
Commit:  7f56908b5a4a12bb97e649cc971a55eec9152f5a → f55cf6bde50cf7dd2e9841a3006646b7c331824a 

Branch pushed to git repo; I updated commit sha1. New commits:
f55cf6b  Merge tag '6.8' into u/dkrenn/18182

comment:7 Changed 7 years ago by
Branch:  u/dkrenn/18182 → u/dkrenn/18182on6.7 

Commit:  f55cf6bde50cf7dd2e9841a3006646b7c331824a → 7f56908b5a4a12bb97e649cc971a55eec9152f5a 
comment:8 Changed 7 years ago by
Work issues:  → merge 6.8 

comment:9 Changed 7 years ago by
Commit:  7f56908b5a4a12bb97e649cc971a55eec9152f5a → cbb0d83f6daca32836b5b05fa9a2f055f05690b9 

Branch pushed to git repo; I updated commit sha1. New commits:
cbb0d83  Merge tag '6.8' into u/dkrenn/18182

comment:10 Changed 7 years ago by
Commit:  cbb0d83f6daca32836b5b05fa9a2f055f05690b9 → 7f56908b5a4a12bb97e649cc971a55eec9152f5a 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
comment:11 Changed 7 years ago by
Branch:  u/dkrenn/18182on6.7 → u/dkrenn/18182on6.8 

Commit:  7f56908b5a4a12bb97e649cc971a55eec9152f5a → cbb0d83f6daca32836b5b05fa9a2f055f05690b9 
Work issues:  merge 6.8 
New commits:
cbb0d83  Merge tag '6.8' into u/dkrenn/18182

comment:12 Changed 7 years ago by
Commit:  cbb0d83f6daca32836b5b05fa9a2f055f05690b9 → 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 followup: 14 Changed 7 years ago by
Status:  needs_info → 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 Changed 7 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 7 years ago by
Commit:  2de49baf67030c02b926fb09e4470027f420bbd4 → 60b9375bd3733f5b0a1a802504b8348a8feb6795 

comment:16 Changed 7 years ago by
Branch:  u/dkrenn/18182on6.8 → u/dkrenn/18182/pushout 

Status:  needs_work → needs_review 
reverted the change in base_ring of category_object (this will be a separate ticket (#19225))
comment:17 Changed 7 years ago by
Status:  needs_review → needs_work 

comment:18 Changed 7 years ago by
Commit:  60b9375bd3733f5b0a1a802504b8348a8feb6795 → 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 7 years ago by
Status:  needs_work → needs_review 

Bug fixed...let's see what the patchbot does...
comment:20 followup: 21 Changed 7 years ago by
Branch:  u/dkrenn/18182/pushout → u/behackl/coercion/pushout 

Commit:  c16587c8b549dde5891f00ec9be10426889c7424 → d449fabb1adcd514bf988e64736a18de34dd7e25 
Reviewers:  Daniel Krenn → Benjamin Hackl, Daniel Krenn 
Status:  needs_review → 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 followup: 24 Changed 7 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 7 years ago by
Branch:  u/behackl/coercion/pushout → u/dkrenn/coercion/pushout 

comment:23 Changed 7 years ago by
Commit:  d449fabb1adcd514bf988e64736a18de34dd7e25 → bc70cb9f592a7f3eb93267b91591d9cd4ae7b358 

Status:  needs_work → needs_review 
comment:24 Changed 7 years ago by
Status:  needs_review → 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 7 years ago by
Branch:  u/dkrenn/coercion/pushout → bc70cb9f592a7f3eb93267b91591d9cd4ae7b358 

Resolution:  → fixed 
Status:  positive_review → closed 
(see also #15425)