#17600: AsymptoticGrowthElement
-------------------------------------+-------------------------------------
Reporter: behackl | Owner:
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-6.9
Component: asymptotic | Resolution:
expansions | Merged in:
Keywords: asymptotics, | Reviewers: Daniel Krenn, Clemens
gsoc2015 | Heuberger
Authors: Benjamin Hackl, | Work issues:
Daniel Krenn | Commit:
Report Upstream: N/A | 2bf68685c1f2c09a258b592d537f34cd9c77b2d3
Branch: | Stopgaps:
u/behackl/asy/growthGroup |
Dependencies: |
-------------------------------------+-------------------------------------
Changes (by behackl):
* status: needs_work => needs_review
Comment:
Replying to [comment:38 cheuberg]:
> - I think that the following behaviour does not conform to the use of
the parameter `category` in `Parent` ("If category is a list or tuple, a
!JoinCategory is created out of them."):
> {{{
> sage: import sage.rings.asymptotic.growth_group as agg
> sage: agg.GenericGrowthGroup(ZZ, category=(Posets(), Groups()))
> Traceback (most recent call last):
> ...
> ValueError: (Category of posets, Category of groups) is not a
subcategory of Join of Category of groups and Category of posets
> }}}
This is because our code checks, whether at least one argument in the
tuple is a subcategory of `Join of Category of groups and Category of
posets`. Of course, this could be relaxed such that we check if there is
at least one argument that is a subcategory of `Category of groups`, and
at least one that is a subcategory of `Category of posets`.
However, we already took care of that in the cleanup ticket #19083. Thus,
I'd leave this as it is here.
> - `GenericGrowthElement` - class documentation: "abstract
implementation" sounds contradictory.
Rephrased to "basic implementation".
> - `GenericGrowthElement.__init__`: first and second tests seem to do the
same thing.
Duplicate removed.
> - `GenericGrowthGroup._element_constructor_`: make clear that either
`data` or `raw_element` has to be given; in the latter case, `data` has to
be 0.
Done.
> - `GenericGrowthGroup._element_constructor_`: shouldn't the tests be
marked as indirect doctests?
Done.
> - `GenericGrowthGroup._element_constructor_`: is there a particular
reason for using `type(data) == self.element_class` in the second line and
`isinstance(data, self.element_class)` in the fourth line of the code?
Not as far as I can remember. I have rewritten this particular part of the
element constructor.
> - `GenericGrowthGroup.gens_monomial`: it is not clear what the
difference between `gens` and `gens_monomial` should be.
I've tried to clarify by extending the documentation.
> - `MonomialGrowthGroup.__classcall__`: The category parameter is not
checked, but I can live with that:
Fixed on the cleanup-ticket.
> - `MonomialGrowthGroup.__init__`: Please include doctests for the two
`ValueErrors`
We put the code handling variables in a separate class on the cleanup
ticket. Thus, I think that this is not really necessary.
> - `MonomialGrowthGroup._convert_`: I think that the error message on
variable in a multivariate power series is not instructive in the
following example, I'd prefer to see "Cannot convert 2" as in `G(2)`
below.
> {{{
> sage: from sage.rings.asymptotic.growth_group import MonomialGrowthGroup
> sage: G = MonomialGrowthGroup(ZZ, 'x')
> sage: R.<x, y> = PowerSeriesRing(ZZ, ['x', 'y'])
> sage: R(2).variables()
> ()
> sage: G(R(2))
> Traceback (most recent call last):
> ...
> NotImplementedError: variable not defined for multivariate power series;
> use 'variables' instead.
> sage: G(2)
> Traceback (most recent call last):
> ...
> ValueError: Cannot convert 2.
> }}}
Fixed.
> - `MonomialGrowthGroup.gens_monomial`: Is this method needed? See the
example:
> {{{
> sage: from sage.rings.asymptotic.growth_group import MonomialGrowthGroup
> sage: MonomialGrowthGroup(ZZ, 'log(x)').gens_monomial()
> ()
> sage: MonomialGrowthGroup(ZZ, 'exp(x)').gens_monomial()
> (exp(x),)
> }}}
I've added an additional note to explain what this method should exactly
do. Note that while it can only recognize `log(...)` on this ticket, we
have extended the functionality of it (to actually do what it should) on
ticket #18587.
> - `MonomialGrowthGroup.gens`: cf. `gens_monomial`: I'd move "even if the
growth group is logarithmic" from `gens` to `gens_monomial` as "except if
the growth group is logarithmic"
Removed the half-sentence. The additional note for `gens_monomial` should
make the difference clear.
> - `parent_to_repr_short`: Rewrite INPUT section to standard format
Done.
Thank you for your review---this is now ready for a short cross-review.
--
Ticket URL: <http://trac.sagemath.org/ticket/17600#comment:42>
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.
