#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: | 67a73cabf434ab2d6642db0b9e5e8eeec9767feb
u/behackl/asy/asymptoticTerm | Stopgaps:
Dependencies: #17600, #18930, |
#19237 |
-------------------------------------+-------------------------------------
Changes (by behackl):
* status: needs_info => needs_review
Comment:
Replying to [comment:36 cheuberg]:
> My impression when reviewing this ticket was that absorption is
explained many times. Perhaps these explanations and examples could have
been collected in one place (either in the module documentation or in the
relevant methods in the base class). Then the documentation of all
overriden classes could have included a link to the general explanation
and only explain the particularities of the respective class. Having the
explanation in the docstring of a method of the base class would have the
advantage that users of `method?` could also access it.
>
> This module is of rather technical nature anyway and the end user will
hopefully rarely look into it. Thus reducing the number of occurrences of
`can_absorb` in the reference manual does not concern me too much.
>
> IMHO, having `can_absorb` as a trivial wrapper around `_can_absorb_` has
a (very slight) performance penalty and makes future reading of the code
somewhat harder.
I think that you are right. I've collected the documentation for `absorb`
and `can_absorb` and put it in a section of the module description. The
respective methods now only have a short documentation (explaining their
own behavior), and a reference to that section.
> >
> > > - `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.
>
> Could you add one sentence to the documentation? Something like "Setting
`check=False` is meant to be used in cases where the truth of `can_absorb`
has been checked in advance to avoid duplicate checking."
Done.
> > > - `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.
>
> I think that if the current code expects the growth group to be a
`GenericGrowthGroup`, then the documentation should state that.
>
It does so now.
> >
> > > - `GenericTermMonoid._element_constructor_`: why example
`T_QQ.coerce(term1)` instead of `T_QQ(term1)`?
> >
> > Changed.
>
> The attached comment should also be changed. Having a look again, I now
partially understand why `coerce` was called in the first place: it seems
to explain why comparison in the line above could work. However, in the
`_element_constructor_` method, I expected to see an example of
conversion.
> Perhaps both could be included: first conversion, then comparison and
the explanation that coercion did actually work.
I don't think that ''conversion'' is at work in this particular section---
in any case, I've rewritten these doctests slightly in order to better
reflect what we want to demonstrate.
>
> > > - `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...
>
> I do not know yet what the usecase for comparison of elements of equal
growth was supposed to be.
> For me, only comparing the growth would be sufficient. Mixing the order
of the growth group and the order of the base ring seems to be some kind
of overkill, if at all possible.
>
> I am not sure, though, whether `x <= 2*x` and `2*x <= x` should hold. Or
what about mixing exact terms and `O` terms.
>
> Are distinct conventions for `_le_` and `can_absorb` necessary?
In fact, we only need the comparison of the underlying growth. I've
deleted `TermWithCoefficient._le_` and rewritten the documentation of
`GenericTerm.__le__` and `GenericTerm._le_` in order to reflect that.
As this is--as you already said--a rather technical class, I think that it
can be justified if `<=` only compares growth. However, if we follow this
approach, then `x <= 2*x` and `2*x <= x` would both yield `True`, but `x
== 2*x` would be false (because as far as I can remember, we need `==` to
actually check if the coefficients are the same too, somewhere in the
`AsymptoticRing`). Any thoughts?
>
> > > - `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.
>
> Could someone elaborate on that?
>
> > Also, I think that having this as a property is overall cleaner.
>
> Why?
>
> Including this ticket, we have 74 classes with `base_ring`,
> {{{
> $rgrep 'def base_ring' | wc
> 74 230 4186
> }}}
> and one of them (this ticket) has it as a property:
> {{{
> $ rgrep -C1 'def base_ring' . |grep property
> ./rings/asymptotic/term_monoid.py- @property
> }}}
>
> > Let me know if you think that we should enforce consistency with the
`PolynomialRing`.
>
> I'd like to see more convincing arguments before breaking consistency
with 73 other sage modules.
>
Mea culpa, I was wrong. Later on, we renamed `base_ring` to
`coefficient_ring`---but as far as I've looked up, we never acutally
**set** the property outside of initialization. So this could easily
become a function (which is what I also changed here).
> > I pushed my changes to this ticket; please cross-review them and let
me know how you want to proceed regarding the open questions.
>
> done.
>
> I have one further question not discussed so far:
>
> - `absorb` uses the coercion framework before calling `_absorb_` with
guaranteed equal parents.
> `can_absorb` does not. Is there a reason for that? Does only the
growth group play a role here,
> so that different parents do not really matter?
Well, we currently only have three cases when `t1.can_absorb(t2)` is
called:
- `t1` is a term from an abstract base class. These cannot absorb
anything, so `False` is returned. No need to ask the coercion framework.
- `t1` is an O`-term. In this case, `can_absorb` tests `t1 <= t2`, which
in turn asks the coercion framework for help.
- `t1` is an exact term. Here, `t2` can be absorbed if and only if `t2` is
an exact term as well, and the growth needs to coincide as well. I don't
think that the coercion framework should be involved in this case.
Benjamin
--
Ticket URL: <http://trac.sagemath.org/ticket/17715#comment:38>
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.
