#18182: pushout construction and finding common parents for/including cartesian
products
-------------------------------------+-------------------------------------
Reporter: dkrenn | Owner:
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-6.6
Component: coercion | Resolution:
Keywords: sd67, coercion, | Merged in:
categories | Reviewers: Benjamin Hackl,
Authors: Daniel Krenn, | Daniel Krenn
David Roe | Work issues:
Report Upstream: N/A | Commit:
Branch: | d449fabb1adcd514bf988e64736a18de34dd7e25
u/behackl/coercion/pushout | Stopgaps:
Dependencies: |
-------------------------------------+-------------------------------------
Changes (by behackl):
* status: needs_review => needs_work
* commit: c16587c8b549dde5891f00ec9be10426889c7424 =>
d449fabb1adcd514bf988e64736a18de34dd7e25
* branch: u/dkrenn/18182/pushout => u/behackl/coercion/pushout
* reviewer: Daniel Krenn => Benjamin Hackl, Daniel Krenn
Comment:
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.
2. `categories.pushout.ConstructionFunctor.common_base`: I think that
`Raise a CoercionException` does not fit in a `OUTPUT`-block from a
semantic point of view.
3. `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.)
4. `MultivariateConstructionFunctor.common_base`: Could you explain why
you use `get_coercion_model().common_parent(...)` instead of
`pushout(...)`?
5. `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.
6. `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.)
7. `pushout`: `CartesianProductPolys` vs. `CartesianProductPoly`? (cf.
#18223 `;-)`).
Benjamin
----
New commits:
||[http://git.sagemath.org/sage.git/commit/?id=5c29fd4a50a40d9cc0fcd5d23288639eda3f35f7
5c29fd4]||{{{fix typos}}}||
||[http://git.sagemath.org/sage.git/commit/?id=6233426582384da8b5935854bbf6beae5976b63f
6233426]||{{{improve language}}}||
||[http://git.sagemath.org/sage.git/commit/?id=d31667e083586a1c3c47dcf98a96ec6b72d558f8
d31667e]||{{{fix ReSt-error}}}||
||[http://git.sagemath.org/sage.git/commit/?id=c4a5bfd0af27ae8aab42f7b51002273ffa46a875
c4a5bfd]||{{{remove superfluous import in doctest, superfluous empty line
in docstring, and fix spacing in a line (pep 8)}}}||
||[http://git.sagemath.org/sage.git/commit/?id=726b74ada27a58d2a899ca2b2fa66fc15b7e9a05
726b74a]||{{{CartesianProductPolys: check whether other has a
construction}}}||
||[http://git.sagemath.org/sage.git/commit/?id=d449fabb1adcd514bf988e64736a18de34dd7e25
d449fab]||{{{add two doctests}}}||
--
Ticket URL: <http://trac.sagemath.org/ticket/18182#comment:20>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
