Opened 4 years ago

Closed 4 years ago

#22828 closed enhancement (fixed)

py3: rich comparison for asymptotic term monoid

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.0
Component: python3 Keywords:
Cc: cheuberg, behackl, dkrenn Merged in:
Authors: Frédéric Chapoton, Daniel Krenn Reviewers: Daniel Krenn, Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 69ac798 (Commits, GitHub, GitLab) Commit: 69ac798f10445e8c30db25707531ef61a0aaba97
Dependencies: Stopgaps:

Status badges

Description (last modified by 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)

Change History (23)

comment:1 Changed 4 years ago by chapoton

  • Branch set to u/chapoton/22828
  • Commit set to 0e790b08aab3898c846faab3f29f6ce45b83df71

work in progress


New commits:

0e790b0rich comparison for asymptotic term monoid

comment:2 Changed 4 years ago by git

  • Commit changed from 0e790b08aab3898c846faab3f29f6ce45b83df71 to 7497d362f39547dff48c526b17561c2836dad761

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

7497d36trac 22828 more work, not yet finished

comment:3 Changed 4 years ago by git

  • Commit changed from 7497d362f39547dff48c526b17561c2836dad761 to 4c8c3180e1e5ab42c3d2df1a188bc14234816339

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

4c8c318trac 22828 even more work, not yet ok

comment:4 Changed 4 years ago by git

  • Commit changed from 4c8c3180e1e5ab42c3d2df1a188bc14234816339 to efd771d1245275d6a02df85d6681e8063dfc2e8e

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

efd771dtrac 22828 work again, not the end

comment:5 Changed 4 years ago by chapoton

  • Cc cheuberg behackl dkrenn added
  • Description modified (diff)

comment:6 Changed 4 years ago by git

  • Commit changed from efd771d1245275d6a02df85d6681e8063dfc2e8e to 366066890786c51cacdfda64edafe82bb2bd9e82

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

3660668trac 22828 fix imports

comment:7 Changed 4 years ago by dkrenn

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

comment:8 Changed 4 years ago by dkrenn

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)?

comment:9 Changed 4 years ago by dkrenn

  • Branch changed from u/chapoton/22828 to u/dkrenn/22828

comment:10 Changed 4 years ago by dkrenn

  • Authors changed from Frédéric Chapoton to Frédéric Chapoton, Daniel Krenn
  • Commit changed from 366066890786c51cacdfda64edafe82bb2bd9e82 to 9b2a790c5cb6ce7ef776056ecba0f31af62ad1bf
  • Reviewers set to Daniel Krenn
  • Status changed from new to needs_review

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:

9a7c310Revert "trac 22828 more work, not yet finished"
24df411Revert "rich comparison for asymptotic term monoid"
d8841fbTrac #22828: richcmp via _lt_ and _eq_
78938a5Trac #22828: re-do and revert reverted clean-up
b33fe1dTrac #22828: rewrite to use _richcmp_
77f9f96Trac #22828: use _richcmp_ in asymptotics_multivariate_generating_functions and get rid of mathematically senseless __lt__
b497604Trac #22828: test that no < comparison is possible
6ada528Trac #22828: newlines according to PEP8
1ee6f0bTrac #22828: update docs of _total_order_key_
9b2a790Trac #22828: write docs and doctests for richcmp_by_eq_and_lt

comment:11 follow-up: Changed 4 years ago by git

  • Commit changed from 9b2a790c5cb6ce7ef776056ecba0f31af62ad1bf to 12eb632931b23787b8dc5597e05ff5bfc91aed0c

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

12eb632Trac #22828: remove meaningless doctest

comment:12 in reply to: ↑ 11 Changed 4 years ago by dkrenn

Replying to git:

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

12eb632Trac #22828: remove meaningless doctest

Is again at needs_review.

comment:13 follow-up: Changed 4 years ago by 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 ?

comment:14 in reply to: ↑ 13 Changed 4 years ago by dkrenn

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.

comment:15 follow-up: Changed 4 years ago by 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.

comment:16 in reply to: ↑ 15 Changed 4 years ago by dkrenn

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...)

comment:17 Changed 4 years ago by chapoton

  • Reviewers changed from Daniel Krenn to Daniel Krenn, Frédéric Chapoton
  • Status changed from needs_review to positive_review

ok, indeed. sorry for the noise.

Then let it be.

comment:18 follow-up: Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work

PDF docs don't build

comment:19 in reply to: ↑ 18 ; follow-up: Changed 4 years ago by dkrenn

  • Status changed from needs_work to needs_info

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?

comment:20 Changed 4 years ago by git

  • Commit changed from 12eb632931b23787b8dc5597e05ff5bfc91aed0c to 69ac798f10445e8c30db25707531ef61a0aaba97

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

bb711ebMerge tag '8.0.beta4' into t/22828/22828
69ac798Trac #22828: correct quotes: `...` by ``...`

comment:21 in reply to: ↑ 19 Changed 4 years ago by dkrenn

  • Status changed from needs_info to needs_review

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.

comment:22 Changed 4 years ago by chapoton

  • Status changed from needs_review to positive_review

ok, then

comment:23 Changed 4 years ago by vbraun

  • Branch changed from u/dkrenn/22828 to 69ac798f10445e8c30db25707531ef61a0aaba97
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.