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: sage-6.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:

Status badges

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 Nathann Cohen

(see also #15425)

comment:2 Changed 7 years ago by David Roe

Branch: u/roed/18182

comment:3 Changed 7 years ago by Daniel Krenn

Branch: u/roed/18182u/dkrenn/18182

comment:4 Changed 7 years ago by git

Commit: 7f56908b5a4a12bb97e649cc971a55eec9152f5a

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

fe83e88update authors of pushout
7f56908fix links in docu

comment:5 Changed 7 years ago by Daniel Krenn

Authors: Daniel Krenn, David Roe
Reviewers: Daniel Krenn
Status: newneeds_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 git

Commit: 7f56908b5a4a12bb97e649cc971a55eec9152f5af55cf6bde50cf7dd2e9841a3006646b7c331824a

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

f55cf6bMerge tag '6.8' into u/dkrenn/18182

comment:7 Changed 7 years ago by Daniel Krenn

Branch: u/dkrenn/18182u/dkrenn/18182-on-6.7
Commit: f55cf6bde50cf7dd2e9841a3006646b7c331824a7f56908b5a4a12bb97e649cc971a55eec9152f5a

comment:8 Changed 7 years ago by Daniel Krenn

Work issues: merge 6.8

comment:9 Changed 7 years ago by git

Commit: 7f56908b5a4a12bb97e649cc971a55eec9152f5acbb0d83f6daca32836b5b05fa9a2f055f05690b9

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

cbb0d83Merge tag '6.8' into u/dkrenn/18182

comment:10 Changed 7 years ago by git

Commit: cbb0d83f6daca32836b5b05fa9a2f055f05690b97f56908b5a4a12bb97e649cc971a55eec9152f5a

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

comment:11 Changed 7 years ago by Daniel Krenn

Branch: u/dkrenn/18182-on-6.7u/dkrenn/18182-on-6.8
Commit: 7f56908b5a4a12bb97e649cc971a55eec9152f5acbb0d83f6daca32836b5b05fa9a2f055f05690b9
Work issues: merge 6.8

New commits:

cbb0d83Merge tag '6.8' into u/dkrenn/18182

comment:12 Changed 7 years ago by git

Commit: cbb0d83f6daca32836b5b05fa9a2f055f05690b92de49baf67030c02b926fb09e4470027f420bbd4

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

3cacc52change recursive pushout to common_parent (to take care of direct coercions)
4662eedbug kind of solved (called now directly by cartesian_product); move doctest
ce20dd9Merge remote-tracking branch 'origin/u/dkrenn/18182-on-6.7' into u/dkrenn/18182-on-6.8
23354dbadd missing test and fix broken tests
2de49baMerge remote-tracking branch 'origin/u/dkrenn/18182-on-6.7' into u/dkrenn/18182-on-6.8

comment:13 in reply to:  5 ; Changed 7 years ago by Daniel Krenn

Status: needs_infoneeds_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 Daniel Krenn

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)

Last edited 7 years ago by Daniel Krenn (previous) (diff)

comment:15 Changed 7 years ago by git

Commit: 2de49baf67030c02b926fb09e4470027f420bbd460b9375bd3733f5b0a1a802504b8348a8feb6795

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

cd17673Merge tag '6.9.beta6' into t/18182/18182-on-6.8
3eefe25correct typo in AUTHORS
5fe52e4fix doctests since name of cartesian product functor has changed
60b9375revert changes in base_ring of category_object and adapt doctests

comment:16 Changed 7 years ago by Daniel Krenn

Branch: u/dkrenn/18182-on-6.8u/dkrenn/18182/pushout
Status: needs_workneeds_review

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

Last edited 7 years ago by Daniel Krenn (previous) (diff)

comment:17 Changed 7 years ago by Daniel Krenn

Status: needs_reviewneeds_work

comment:18 Changed 7 years ago by git

Commit: 60b9375bd3733f5b0a1a802504b8348a8feb6795c16587c8b549dde5891f00ec9be10426889c7424

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

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

comment:19 Changed 7 years ago by Daniel Krenn

Status: needs_workneeds_review

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

comment:20 Changed 7 years ago by Benjamin Hackl

Branch: u/dkrenn/18182/pushoutu/behackl/coercion/pushout
Commit: c16587c8b549dde5891f00ec9be10426889c7424d449fabb1adcd514bf988e64736a18de34dd7e25
Reviewers: Daniel KrennBenjamin Hackl, Daniel Krenn
Status: needs_reviewneeds_work

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.

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

Benjamin


New commits:

5c29fd4fix typos
6233426improve language
d31667efix ReSt-error
c4a5bfdremove superfluous import in doctest, superfluous empty line in docstring, and fix spacing in a line (pep 8)
726b74aCartesianProductPolys: check whether other has a construction
d449fabadd two doctests

comment:21 in reply to:  20 ; Changed 7 years ago by Daniel Krenn

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.

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

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

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

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

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

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

Done.

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

Last edited 7 years ago by Daniel Krenn (previous) (diff)

comment:22 Changed 7 years ago by Daniel Krenn

Branch: u/behackl/coercion/pushoutu/dkrenn/coercion/pushout

comment:23 Changed 7 years ago by Daniel Krenn

Commit: d449fabb1adcd514bf988e64736a18de34dd7e25bc70cb9f592a7f3eb93267b91591d9cd4ae7b358
Status: needs_workneeds_review

New commits:

4619fe7Trac #18182, comment 20, 2: rewrite OUTPUT-block
bc70cb9Trac #18182, comment 20, 7: rename CartesianProductPolys --> CartesianProductPoly

comment:24 in reply to:  21 Changed 7 years ago by Benjamin Hackl

Status: needs_reviewpositive_review

Replying to dkrenn:

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

This is now #19375.

Perfect.

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

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

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

comment:25 Changed 7 years ago by Volker Braun

Branch: u/dkrenn/coercion/pushoutbc70cb9f592a7f3eb93267b91591d9cd4ae7b358
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.