#17715: AsymptoticTerm
-------------------------------------+-------------------------------------
Reporter: behackl | Owner:
Type: enhancement | Status: needs_info
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 cheuberg):
* status: needs_review => needs_info
Comment:
Replying to [comment:35 behackl]:
> > - `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?
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.
>
> > - `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."
> > - `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.
>
> > - `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.
> > - `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?
> > - `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.
> 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?
--
Ticket URL: <http://trac.sagemath.org/ticket/17715#comment:36>
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.
