py3: rich comparison for asymptotic term monoid

[Frédéric Chapoton]

as another step towards python3

The asymptotic/ folder needs a clean-up of its handling of comparison, for correct insertion in the coercion framework and compatibility with python3 (use of rich comparison)

[Frédéric Chapoton]

work in progress

---

New commits:

​0e790b0

rich comparison for asymptotic term monoid

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​7497d36

trac 22828 more work, not yet finished

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​4c8c318

trac 22828 even more work, not yet ok

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​efd771d

trac 22828 work again, not the end

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​3660668

trac 22828 fix imports

[Daniel Krenn]

I'll have a look at the changes in the next days.

[Daniel Krenn]

A quick question ahead: Why is something like

def _richcmp_(self, other, op):
    if not isinstance(other, GenericGrowthElement):
        return op == op_NE

needed? Shouldn't _richcmp_ always get elements with the same parent (by the coercion model), thus elements that are already directly comparable (and the code not needed)?

[Daniel Krenn]

Dear Frédéric, I've reviewed your changes, but see comments below this list for a possible future of this ticket:

• Some doctests did not pass (see also the following comments); however, as this ticket was not set to "needs review", I assume that these were just not investigated yet.

• asymptotics_multi...: LGTM, but see comment below this list.

• growth_group.py, line 1196:

  return richcmp(self._raw_element_, other._raw_element_, op)
  

This does not make sense for a GenericGrowthElement (as there is no < on it defined).

• growth_group.py, line 3643: The following was rewritten, but the new code is not equivalent to the old:

          return bool(abs(self.base) < abs(other.base)) or \
                 bool(self.base == other.base)
  

vs

        if not isinstance(other, ExponentialGrowthElement):
            return op == op_NE

        lx = self.base
        ly = other.base
        if lx == ly:
            return rich_to_bool(op, 0)

        return richcmp(abs(lx), abs(ly), op)

For example: self.base=1, other.base=-1, op=op_LE would incorrectly return True. I think, that this is the origin of the failing doctests.

• term_monoid.py, lines 2915-2933: Maybe a matter of taste, but it looks like the code has blown up quite much (there is even some code doubling); this does not improve readability.

• The patchbot reports also this:

  File "src/sage/calculus/calculus.py", line 1406, in sage.calculus.calculus.laplace
  Failed example:
      laplace(t^n, t, s, algorithm='giac')
  Expected:
      Traceback (most recent call last):
      ...
      NotImplementedError: Unable to parse Giac output: integrate(t^n*exp(-s*t),t,0,+infinity)
  Got:
      integration(t^n*e^(-s*t), t, 0, +Infinity)
  

I have no glue why this is failing. I cannot imagine that this comes from the changes of this ticket. Any ideas?

When the asymptotic expansion module was written, I had the change in comparison in my mind and tried to prepare everything for it, so that it will be easy at one point. This point seems to be now; I've partially reverted your changes and have prepared a own patch to address the issues of this ticket. Overall, the code now is much cleaner (compared to the original code) and I am satisified with it.

• Can you please check: Is this is now still Python3 comparision-compatible? (I think so, but it is surely easier for you to tell. ;)

• The main change was to introduce a kind of low-level _richcmp_ which can be used more broadly. As a consequence only high-level _eq_ and _lt_ (very close to the Python 3 recommendation on how to implement comparison, but single underscores to use SageMath's convention on coercion) have to be implemented. I think this is a very convenient to implement new partial orders.

• In asymptotics_multi..., I've deleted the the comparison <, as it is mathematically not meaningful and only needed in filtering out multiple factors in the denominator. There is now used a key, where this sorting is needed.

---

Last 10 new commits:

​9a7c310	Revert "trac 22828 more work, not yet finished"
​24df411	Revert "rich comparison for asymptotic term monoid"
​d8841fb	Trac #22828: richcmp via _lt_ and _eq_
​78938a5	Trac #22828: re-do and revert reverted clean-up
​b33fe1d	Trac #22828: rewrite to use _richcmp_
​77f9f96	Trac #22828: use _richcmp_ in asymptotics_multivariate_generating_functions and get rid of mathematically senseless __lt__
​b497604	Trac #22828: test that no < comparison is possible
​6ada528	Trac #22828: newlines according to PEP8
​1ee6f0b	Trac #22828: update docs of _total_order_key_
​9b2a790	Trac #22828: write docs and doctests for richcmp_by_eq_and_lt

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​12eb632

Trac #22828: remove meaningless doctest

[Daniel Krenn]

Replying to git:

Branch pushed to git repo; I updated commit sha1. New commits:

​12eb632

Trac #22828: remove meaningless doctest

Is again at needs_review.

[Frédéric Chapoton]

Probably you should not care about the rubik failure, it should be a random thing.

Could you please check that all tests pass in the asymptotic folder with this branch + the current branch of ticket #22297 ?

[Daniel Krenn]

Replying to chapoton:

Probably you should not care about the rubik failure, it should be a random thing.

I would guess so. I just find random failing doctests not very pleasing (from a global-SageMath-point-of-view).

Could you please check that all tests pass in the asymptotic folder with this branch + the current branch of ticket #22297 ?

Tests passed.

[Frédéric Chapoton]

looks ok, but you should not do that:

-from __future__ import absolute_import

please add them back (at least twice), then you can set a positive review on my behalf.

[Daniel Krenn]

Replying to chapoton:

looks ok, but you should not do that:

-from __future__ import absolute_import

please add them back (at least twice), then you can set a positive review on my behalf.

They are just moved below the copyright notice. (I've taken this change just as it was in the original patch provided by you...)

[Frédéric Chapoton]

ok, indeed. sorry for the noise.

Then let it be.

[Volker Braun]

PDF docs don't build

[Daniel Krenn]

Replying to vbraun:

PDF docs don't build

This is not caused by this ticket. Building the Pdf (tried for reference/asymptotic) fails in a clean 8.0.beta4. Moreover, it fails already in 8.0.beta2, but seems to work in 7.6. What has changed?

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​bb711eb	Merge tag '8.0.beta4' into t/22828/22828
​69ac798	Trac #22828: correct quotes: `...` by ``...`

[Daniel Krenn]

Replying to dkrenn:

Replying to vbraun:

PDF docs don't build

This is not caused by this ticket. Building the Pdf (tried for reference/asymptotic) fails in a clean 8.0.beta4. Moreover, it fails already in 8.0.beta2, but seems to work in 7.6. What has changed?

Ok, as this iftex-issue is now not anymore on my machine, I saw the true error (it is related to this ticket, sorry for my wrong assertion above) and changed it.

[Frédéric Chapoton]

ok, then