#18587: cartesian products of growth groups
-------------------------------------+-------------------------------------
Reporter: dkrenn | Owner:
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-6.9
Component: asymptotic | Resolution:
expansions | Merged in:
Keywords: asymptotics, | Reviewers: Clemens Heuberger
gsoc15 | Work issues:
Authors: Benjamin Hackl, | Commit:
Daniel Krenn | 6fefead5a7fd8cfe4e84d73641b4459c4671ceb4
Report Upstream: N/A | Stopgaps:
Branch: u/cheuberg/asy |
/growth-group-cartesian |
Dependencies: #18223, #18586, |
#18930 |
-------------------------------------+-------------------------------------
Changes (by cheuberg):
* status: needs_review => needs_work
* commit: 4a6174f7e86903243009162bd2e0b6bf6120c2b2 =>
6fefead5a7fd8cfe4e84d73641b4459c4671ceb4
* reviewer: => Clemens Heuberger
* milestone: sage-6.8 => sage-6.9
Comment:
I reviewed this ticket, but without reviewing the dependency on #18223, so
consider this as a conditional review.
I added a few reviewer commits. There is still a merge conflict with
6.9.rc0 caused by #17715.
Here are my other comments.
1. Introduction
- I suggest to add a section heading before the note box (e.g. "Creation
of Growth Groups") and to downgrade the note box to be an introductory
paragraph of this section.
- Last sentence: "It is mathematically nonsense": explain how the order
of factors of a single variable matters.
2. `Variable.__init__`: `ValueError` "is not a valid name": not tested
3. `Variable.extract_variable_names`:
- I am surprised that a more or less complete symbolic expression
parser is needed here. Would it not be simpler to use `SR(s)` and then
call `.variables()`, possibly on subexpressions in order to detect
duplicates? It seems that `SR(s)` would create the symbolic variables on
the fly, but does not inject them into the global name space, so this
seems to be what is needed here.
- I do not think the following is intentional:
{{{
sage: Variable.extract_variable_names('a + (b + c) + d')
('b', 'c', 'd')
}}}
- Why does the regular expression for number contain a `$` but no `^`?
4. `GenericGrowthGroup.gens_monomial`: Replace "is only implemented for"
by "must be implemented in"
5. `MonomialGrowthGroup`: Is the following intentionally allowed:
{{{
sage: from sage.rings.asymptotic.growth_group import MonomialGrowthGroup
sage: MonomialGrowthGroup(ZZ, 'x+y')
Growth Group x+y^ZZ
}}}
6. `MonomialGrowthGroup._convert_`:
- Is the following intentional:
{{{
sage: import sage.rings.asymptotic.growth_group as agg
sage: P = agg.MonomialGrowthGroup(ZZ, 'x')
sage: P(1)
1
sage: P('x^2')
x^2
sage: P('1')
Traceback (most recent call last):
...
ValueError: Cannot convert 1.
sage: G('x^)42(')
x^42
}}}
- As in 3a, couldn't the string parsing be done by the symbolic ring?
7. `GrowthGroupFactory.create_key_and_extra_argument`: Again, my
suggestion is to delegate the string parsing to the symbolic ring. Note
that `SR('x^ZZ')` happily accepts `ZZ` as a variable, so I imagine that
postprocessing here would not involve big problems. I do not read this
string parser here before the decision on its eventual fate.
8. Add one or more links from `growth_group.py` to
`growth_group_cartesian.py`.
9. `CartesianProductFactory`:
- indicate (somewhere, probably at a more prominent place) what order
is chosen automatically (apparently, lexicographic if the variables are
equal, component-wise otherwise). That would also explain why order does
matter (cf. 1b).
- the example with "do not have disjoint variables" is somewhat
puzzeling, because the cartesian product of `A` and `B` was allowed.
- (continuation of b while reading the code): apparently, sets of
variables of the factors must be equal or disjoint.
- mark doctests as indirect
10. `GenericProduct`: mark doctests as indirect.
11. Check links `~sage.sets.cartesian_product.CartesianProductPosets` and
`sage.sets.cartesian_product.CartesianProductPosets.Element` (do not work
for me, possibly time intensive recreation of documentation would help).
12. `GenericProduct._coerce_map_from_`: don't the factors coerce into the
product?
{{{
sage: from sage.rings.asymptotic.growth_group import GrowthGroup
sage: G = GrowthGroup('x^ZZ')
sage: H = GrowthGroup('log(x)^ZZ')
sage: K = cartesian_product([G, H])
sage: K.has_coerce_map_from(G)
False
sage: K.has_coerce_map_from(H)
False
}}}
13. Expand documentation of `UnivariateProduct` and `MultivariateProduct`:
at least, include a link to `GenericProduct` and explain the choice of
order.
----
New commits:
||[http://git.sagemath.org/sage.git/commit/?id=59430d8ff60a44ec5442ae4144d9754b0c98b19c
59430d8]||{{{Trac #18587: remove redundant doctest}}}||
||[http://git.sagemath.org/sage.git/commit/?id=f11377d6c4d8c64c75932ebdde7d7a7fa5ae58a9
f11377d]||{{{Trac #18587: minor language issues}}}||
||[http://git.sagemath.org/sage.git/commit/?id=894e3a83ceb1b2f0cb4f508f57ca5bce8c84c62a
894e3a8]||{{{Trac #18587: Fix ReSt errors}}}||
||[http://git.sagemath.org/sage.git/commit/?id=61085d0aa73893bb9439f10c53466fc3fe872658
61085d0]||{{{Trac #18587: add heading "Classes and Methods"}}}||
||[http://git.sagemath.org/sage.git/commit/?id=6fefead5a7fd8cfe4e84d73641b4459c4671ceb4
6fefead]||{{{Trac #18587: Fix minor errors}}}||
--
Ticket URL: <http://trac.sagemath.org/ticket/18587#comment:18>
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.
