#17715: AsymptoticTerm
-------------------------------------+-------------------------------------
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
gsoc15 | Heuberger
Authors: Benjamin Hackl | Work issues:
Report Upstream: N/A | Commit:
Branch: | 0bac3bb892aab1e2ae42a20b9f50458fa36acd13
u/behackl/asy/asymptoticTerm | Stopgaps:
Dependencies: #17600, #18930, |
#19237 |
-------------------------------------+-------------------------------------
Changes (by behackl):
* status: needs_work => needs_review
* commit: 347f080df240102799ded90067cbd01beaa386a4 =>
0bac3bb892aab1e2ae42a20b9f50458fa36acd13
* branch: u/cheuberg/asy/asymptoticTerm => u/behackl/asy/asymptoticTerm
Comment:
Hello Clemens!
Replying to [comment:34 cheuberg]:
> I reviewed this ticket, added a few reviewer commits and have a few
comments:
> - `absorption`: One-Sentence-Description should describe what this
function ''does''. What happens if the elements are not comparable
(documentation; probably only possible in a follow-up ticket)?
I adapted the short description and slightly adapted the code, such that a
different !ArithmeticError is raised. (This **should** not introduce a
merge conflict, this code did not change up to (and including) the cleanup
ticket #19083.)
> - `can_absorb`: One-Sentence-Description, then NOTE section would be
superfluous.
Done.
> - `GenericTerm.__init__`: Provide doctests for error conditions
I did introduce some tests -- however, only for those errors that are also
raised on the cleanup ticket #19083 (... one less thing to think about
when merging the changes into there). If you would prefer a full error
coverage on this ticket as well, let me know.
> - `GenericTerm.can_absorb` and `GenericTerm._can_absorb_`: Why ''two''
methods?
As far as I can remember, we wanted `can_absorb` to be a publicly
accessible function, but in the documentation it should not apper too
often. Thus, we wrote one public (well-documented) function that calls
`_can_absorb_`, and implemented `_can_absorb_` in the derived classes
only.
In any case, I suppose we could change that, as we documented all the
"technical" `_can_absorb_`-functions also quite well. What do you think
would be cleaner?
> - `GenericTerm.absorb`: what happens if `check` is not set? Why would
one want to call that?
At some point we wanted an option to surpress this check because in the
`AsymptoticRing`, the `MutablePoset` calls `can_absorb` really **very**
often, which makes it a bottleneck, up to some degree. In certain cases,
for example when adding an `O`-term into a `MutablePoset`, then the poset
first needs to do some comparisons in order to place the `O`-term
correctly---and then, in a second step, the `O`-term absorbs all
predecessors (without additional comparison!), because `O`-terms can
absorb anything with weaker growth.
This was the motivation to have some option that allows to skip the test.
Actually doing this optimization is--however--still on our TODO-list.
> - `GenericTerm.absorb`, last 3 lines of code: isn't it quite confusing
to override `self` and `other` in the `lambda` expression?
Yes, it really is. Changed to `left` and `right`.
> - `GenericTerm._absorb_`: the method is never called in the doctests, in
particular, the `NotImplementedError` does not show up.
Added a doctest that directly calls `GenericTerm._absorb_`.
> - `GenericTermMonoid.__init__`: see
[http://trac.sagemath.org/ticket/17600#comment:40 #17600, comment 40]:
handling of tuples as parameter `category`, `__classcall__` for parameter
`category`; same question for derived classes?
Like with the `GrowthGroup`, this is already fixed in #19083. Thus, I'd
leave this as is.
> - `GenericTermMonoid.__init__`: documentation says that `growth_group`
has to be a "partially ordered group" with a `GenericGrowthGroup` only
listed as an example; code explicitly requires a `GenericGrowthGroup`.
This has been partially changed in #19083. There, the growth group only
has to be a parent -- but we should probably also check, whether the
category of the passed growth group is a subcategory of `Posets()`. This
would also fit to the suggested changes in #18223.
In any case, as this ticket enforces the growth group to be derived from
`GenericGrowthGroup`, the category is also a subcategory of `Posets()`.
Thus, I'd also vote for leaving this as is.
> - `GenericTermMonoid._element_constructor_`: why example
`T_QQ.coerce(term1)` instead of `T_QQ(term1)`?
Changed.
> - `GenericTermMonoid._element_constructor_`: why `type(data) ==
self.element_class` and not `isinstance(data, self.element_class)` ?
Similarly for other parents.
Changed.
> - `OTermMonoid._coerce_map_from_`: documentation does not agree with
code: according to the documentation, `S` must be an `OTermMonoid` or an
`ExactTermMonoid`. However, the code does not enforce the former.
Yes, it does---however, it's not really obvious. Calling
{{{
super(OTermMonoid, self)._coerce_map_from_(S)
}}}
enforces this, as we check `isinstance(S, self.__class__)` there.
> - `TermWithCoefficient._le_`: The comparison of coefficients is not
documented. Apart from that, do we really want that `x <= 2*I*x` when the
coefficient ring is complex?
This is problematic. For now, I've included a check for the imaginary part
---but if the coefficient ring is not ordered, this still causes problems.
I suppose that we could discuss removing `_le_` for these terms
altogether, or make terms of equal growth always incomparable: terms of
equal growth are not handled via comparison, but via absorption...
> - `TermWithCoefficientMonoid._init_`: check error condition
Rewritten. Now checks whether `base_ring` is in `Rings()`.
> - `TermWithCoefficientMonoid.base_ring`: why is this a property instead
of a function? Compare with ring of polynomials.
I believe that this makes some of the code in #19083 that handles the
pushout-stuff (... changing the base ring ...) easier. Also, I think that
having this as a property is overall cleaner.
Let me know if you think that we should enforce consistency with the
`PolynomialRing`.
> - `TermWithCoefficientMonoid._coerce_map_from_`: documentation mentions
"this exact term monoid", but this is a `TermWithCoefficientMonoid`.
Fixed.
> - `ExactTerm._repr_`: I do not like the check `self.growth._raw_element_
== 0`, `self.growth` is the identity element of that group, so we should
check for that.
Fixed in #19083.
> - I'd prefer `.is_zero()` over `== 0`.
Changed everywhere in #19083.
> - `TermMonoidFactory.create_key_and_extra_args`: test error conditions
Added some more tests.
I pushed my changes to this ticket; please cross-review them and let me
know how you want to proceed regarding the open questions.
Thanks for the review!
Benjamin
----
Last 10 new commits:
||[http://git.sagemath.org/sage.git/commit/?id=3b0a678934f1e4a71b74a210e086683a5f82b781
3b0a678]||{{{improve absorb, can_absorb}}}||
||[http://git.sagemath.org/sage.git/commit/?id=5aa190e37bb3ebf998e1769733c72e9e921580d8
5aa190e]||{{{test for checking parent of growth in __init__}}}||
||[http://git.sagemath.org/sage.git/commit/?id=ea89a5334c6702cb0d77f65dd2ebc89b80a30d12
ea89a53]||{{{lambda: self, other --> left, right}}}||
||[http://git.sagemath.org/sage.git/commit/?id=d6de96c1f7756ac838a792660713e083b1d3a51c
d6de96c]||{{{test _absorb_}}}||
||[http://git.sagemath.org/sage.git/commit/?id=cf9f0073640d57bea23a0521fe2ca1c438c0991e
cf9f007]||{{{use call instead of .coerce(..)}}}||
||[http://git.sagemath.org/sage.git/commit/?id=6bfb92fa13411dae559730291d423d6ba672fc3b
6bfb92f]||{{{type(..) == ... --> isinstance(...)}}}||
||[http://git.sagemath.org/sage.git/commit/?id=483f6b9dac0187b909ff7210d6d5bebcd7e0d175
483f6b9]||{{{fix comparison of terms with complex coefficients}}}||
||[http://git.sagemath.org/sage.git/commit/?id=19208654b85d58d2c35c35225f9e3590bccfaa4b
1920865]||{{{fix docstring of _coerce_map_from_ from term with coefficient
monoid}}}||
||[http://git.sagemath.org/sage.git/commit/?id=1217348961e26a7f93dcb5df3b7eb0ec71f9f192
1217348]||{{{improve handling of base_ring for term with coefficient
monoid}}}||
||[http://git.sagemath.org/sage.git/commit/?id=0bac3bb892aab1e2ae42a20b9f50458fa36acd13
0bac3bb]||{{{add more tests for TermMonoidFactory}}}||
--
Ticket URL: <http://trac.sagemath.org/ticket/17715#comment:35>
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.
