Opened 5 years ago

Closed 4 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) 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 5 years ago by ncohen

(see also #15425)

comment:2 Changed 5 years ago by roed

  • Branch set to u/roed/18182

comment:3 Changed 4 years ago by dkrenn

  • Branch changed from u/roed/18182 to u/dkrenn/18182

comment:4 Changed 4 years ago by git

  • Commit set to 7f56908b5a4a12bb97e649cc971a55eec9152f5a

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

fe83e88update authors of pushout
7f56908fix links in docu

comment:5 follow-up: Changed 4 years ago by dkrenn

  • Authors set to Daniel Krenn, David Roe
  • 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 4 years ago by git

  • Commit changed from 7f56908b5a4a12bb97e649cc971a55eec9152f5a to f55cf6bde50cf7dd2e9841a3006646b7c331824a

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

f55cf6bMerge tag '6.8' into u/dkrenn/18182

comment:7 Changed 4 years ago by dkrenn

  • Branch changed from u/dkrenn/18182 to u/dkrenn/18182-on-6.7
  • Commit changed from f55cf6bde50cf7dd2e9841a3006646b7c331824a to 7f56908b5a4a12bb97e649cc971a55eec9152f5a

comment:8 Changed 4 years ago by dkrenn

  • Work issues set to merge 6.8

comment:9 Changed 4 years ago by git

  • Commit changed from 7f56908b5a4a12bb97e649cc971a55eec9152f5a to cbb0d83f6daca32836b5b05fa9a2f055f05690b9

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

cbb0d83Merge tag '6.8' into u/dkrenn/18182

comment:10 Changed 4 years ago by git

  • 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 4 years ago by dkrenn

  • 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:

cbb0d83Merge tag '6.8' into u/dkrenn/18182

comment:12 Changed 4 years ago by git

  • Commit changed from cbb0d83f6daca32836b5b05fa9a2f055f05690b9 to 2de49baf67030c02b926fb09e4470027f420bbd4

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 ; follow-up: Changed 4 years ago by dkrenn

  • 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 4 years ago by dkrenn

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 4 years ago by dkrenn (previous) (diff)

comment:15 Changed 4 years ago by git

  • Commit changed from 2de49baf67030c02b926fb09e4470027f420bbd4 to 60b9375bd3733f5b0a1a802504b8348a8feb6795

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 4 years ago by dkrenn

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

Last edited 4 years ago by dkrenn (previous) (diff)

comment:17 Changed 4 years ago by dkrenn

  • Status changed from needs_review to needs_work

comment:18 Changed 4 years ago by git

  • Commit changed from 60b9375bd3733f5b0a1a802504b8348a8feb6795 to c16587c8b549dde5891f00ec9be10426889c7424

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

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

comment:19 Changed 4 years ago by dkrenn

  • Status changed from needs_work to needs_review

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

comment:20 follow-up: Changed 4 years ago by behackl

  • 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:

  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 ; follow-up: Changed 4 years ago by dkrenn

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 4 years ago by dkrenn (previous) (diff)

comment:22 Changed 4 years ago by dkrenn

  • Branch changed from u/behackl/coercion/pushout to u/dkrenn/coercion/pushout

comment:23 Changed 4 years ago by dkrenn

  • Commit changed from d449fabb1adcd514bf988e64736a18de34dd7e25 to bc70cb9f592a7f3eb93267b91591d9cd4ae7b358
  • Status changed from needs_work to needs_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 4 years ago by behackl

  • Status changed from needs_review to positive_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 4 years ago by vbraun

  • Branch changed from u/dkrenn/coercion/pushout to bc70cb9f592a7f3eb93267b91591d9cd4ae7b358
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.