Opened 7 years ago
Closed 7 years ago
#18182 closed enhancement (fixed)
pushout construction and finding common parents for/including cartesian products
Reported by: | dkrenn | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.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 7 years ago by
- Branch changed from u/roed/18182 to u/dkrenn/18182
comment:4 Changed 7 years ago by
- Commit set to 7f56908b5a4a12bb97e649cc971a55eec9152f5a
comment:5 follow-up: ↓ 13 Changed 7 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 7 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 7 years ago by
- Branch changed from u/dkrenn/18182 to u/dkrenn/18182-on-6.7
- Commit changed from f55cf6bde50cf7dd2e9841a3006646b7c331824a to 7f56908b5a4a12bb97e649cc971a55eec9152f5a
comment:8 Changed 7 years ago by
- Work issues set to merge 6.8
comment:9 Changed 7 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 7 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 7 years ago by
- Branch changed from u/dkrenn/18182-on-6.7 to u/dkrenn/18182-on-6.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 7 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 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
|
comment:13 in reply to: ↑ 5 ; follow-up: ↓ 14 Changed 7 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 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 has an idea?
comment:15 Changed 7 years ago by
- Commit changed from 2de49baf67030c02b926fb09e4470027f420bbd4 to 60b9375bd3733f5b0a1a802504b8348a8feb6795
comment:16 Changed 7 years ago by
- Branch changed from u/dkrenn/18182-on-6.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 7 years ago by
- Status changed from needs_review to needs_work
comment:18 Changed 7 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 7 years ago by
- Status changed from needs_work to needs_review
Bug fixed...let's see what the patchbot does...
comment:20 follow-up: ↓ 21 Changed 7 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 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 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 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
|
comment:21 in reply to: ↑ 20 ; follow-up: ↓ 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 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 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.
Cross-reviewed your changes...seem to be fine.
comment:22 Changed 7 years ago by
- Branch changed from u/behackl/coercion/pushout to u/dkrenn/coercion/pushout
comment:23 Changed 7 years ago by
- Commit changed from d449fabb1adcd514bf988e64736a18de34dd7e25 to bc70cb9f592a7f3eb93267b91591d9cd4ae7b358
- Status changed from needs_work to needs_review
comment:24 in reply to: ↑ 21 Changed 7 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 cross-checked 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 changed from u/dkrenn/coercion/pushout to bc70cb9f592a7f3eb93267b91591d9cd4ae7b358
- Resolution set to fixed
- Status changed from positive_review to closed
(see also #15425)