#19083: AsymptoticRing: cleanup, some improvements, documentation
-------------------------------------+-------------------------------------
Reporter: dkrenn | Owner:
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-6.10
Component: asymptotic | Resolution:
expansions | Merged in:
Keywords: | Reviewers: Clemens Heuberger
Authors: Daniel Krenn | Work issues:
Report Upstream: N/A | Commit:
Branch: | 2c60570eb1755791db1a82aa3e3ab70ddbb505c0
u/cheuberg/asy/prototype | Stopgaps:
Dependencies: #19094 |
-------------------------------------+-------------------------------------
Comment (by cheuberg):
Here are my remaining comments (files
`src/sage/rings/asymptotic/asymptotic_ring.py` and
`src/sage/rings/big_oh.py`):
31. `AsymptoticExpansion.__init__`:
- parameter `convert` is neither documented nor tested.
- test error "Cannot include ..."
32. `AsymptoticExpansion._mul_term_`: I'd prefer to say `simplify = not
isinstance(term, ExactTerm)` because this seems to be more robust with
respect to future extensions.
33. `AsymptoticExpansion.__invert__`:
- test errors
- I think that there is shared code between `__invert__`,
`__log__`, `__exp__` and powers with rational exponents. In all those
cases, it is important to split into main term and renormalized remainder.
The main term is then processed according to the respective method, the
remainder is inserted into a converging taylor series with certain
coefficients (this could be handeled by one method getting the sequence of
taylor coefficients as an argument). Possibly in a follow-up ticket.
34. `AsymptoticExpansion.__pow__`:
- why is `(y^2 + O(y))^(1/2)` not tested (add a comment?)
- test error: "Cannot take ... to the negative exponent"
- test error: "Taking ... to the exponent ... not implemented"
- rational exponents could be implemented via binomial series
(follow-up ticket?)
35. `AsymptoticExpansion.log`:
- test error ("several maximal elements")
- I am not convinced that having `k` as an `AsymptoticExpansion`
is very efficient (let the division be in `QQ` and convert later?, cf.
`rpow`.)
36. `AsymptoticExpansion.rpow`:
- Decrease indentation of ALGORITHM block
- The algorithm given in the documentation seems to be quite
obvious
- I do not like "the rest", "the remaining part": those are
actually the asymptotic main parts.
- Why is the algorithm described in that much detail here and
nowhere (`log`, `exp`, `pow`) else?
- Test error "Cannot construct the power ..."
37. `AsymptoticRing`: Add some comment on the "TESTS" section which is
currently inactive.
38. `AsymptoticRing.change_parameter`: is this a good name (the method
sounds like an inplace change, but usually returns a new parent instead)
39. `AsymptoticRing._create_element_via_parent_`:
- I do not like the name and the description of the method.
- The role of `old_parent` is not documented
- In all uses of the method in this module, `old_parent` is set
and is a term monoid. In the example of this method, it is an asymptotic
ring.
- I do not understand why `old_parent` is needed: if
`term.parent() is old_parent`, then `self.change_parameter` will probably
not change the parent anyway.
40. `AsymptoticRing._element_constructor_`:
- I do not understand the sentence "If set, then it is assured
that the terms belong to this asymptotic ring (by converting them if
needed)." Who does the converting?
- The note box seems to be outdated, as the parameter `summands`
no longer exists.
- test error "Not all list entries ..."
- Why `not data or data == 0` instead of `not data`?
- test error "Symbolic expression ... is not in ..."
- What happens with an O-Term in SR?
- conversion of multivariate polynomial ring is not tested.
41. `AsymptoticRing._create_exact_summand_`: Why is this needed and not
handeled by `.create_summand`?
42. `AsymptoticRing.create_summand`:
- doctest case of interesting `data` (i.e. with coefficient and
growth element).
- The following is surprising:
{{{
sage: AR.<z> = AsymptoticRing('z^QQ', QQ)
sage: AR.create_summand('exact', growth='z^2')
z^2
}}}
I'd rather expect an error or zero than using an implicit
coefficient of `1`.
43. Delete the following in `src/doc/en/reference/rings/index.rst`
{{{
.. toctree::
:hidden:
asymptotic_expansions_index
}}}
--
Ticket URL: <http://trac.sagemath.org/ticket/19083#comment:66>
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.
