Opened 5 years ago
Closed 5 years ago
#22828 closed enhancement (fixed)
py3: rich comparison for asymptotic term monoid
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage8.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: 
Description (last modified by )
as another step towards python3
The asymptotic/ folder needs a cleanup 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 5 years ago by
 Branch set to u/chapoton/22828
 Commit set to 0e790b08aab3898c846faab3f29f6ce45b83df71
comment:2 Changed 5 years ago by
 Commit changed from 0e790b08aab3898c846faab3f29f6ce45b83df71 to 7497d362f39547dff48c526b17561c2836dad761
Branch pushed to git repo; I updated commit sha1. New commits:
7497d36  trac 22828 more work, not yet finished

comment:3 Changed 5 years ago by
 Commit changed from 7497d362f39547dff48c526b17561c2836dad761 to 4c8c3180e1e5ab42c3d2df1a188bc14234816339
Branch pushed to git repo; I updated commit sha1. New commits:
4c8c318  trac 22828 even more work, not yet ok

comment:4 Changed 5 years ago by
 Commit changed from 4c8c3180e1e5ab42c3d2df1a188bc14234816339 to efd771d1245275d6a02df85d6681e8063dfc2e8e
Branch pushed to git repo; I updated commit sha1. New commits:
efd771d  trac 22828 work again, not the end

comment:5 Changed 5 years ago by
 Cc cheuberg behackl dkrenn added
 Description modified (diff)
comment:6 Changed 5 years ago by
 Commit changed from efd771d1245275d6a02df85d6681e8063dfc2e8e to 366066890786c51cacdfda64edafe82bb2bd9e82
Branch pushed to git repo; I updated commit sha1. New commits:
3660668  trac 22828 fix imports

comment:7 Changed 5 years ago by
I'll have a look at the changes in the next days.
comment:8 Changed 5 years ago by
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 5 years ago by
 Branch changed from u/chapoton/22828 to u/dkrenn/22828
comment:10 Changed 5 years ago by
 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 29152933: 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 comparisioncompatible? (I think so, but it is surely easier for you to tell. ;)
 The main change was to introduce a kind of lowlevel
_richcmp_
which can be used more broadly. As a consequence only highlevel_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: redo and revert reverted cleanup

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

comment:11 followup: ↓ 12 Changed 5 years ago by
 Commit changed from 9b2a790c5cb6ce7ef776056ecba0f31af62ad1bf to 12eb632931b23787b8dc5597e05ff5bfc91aed0c
Branch pushed to git repo; I updated commit sha1. New commits:
12eb632  Trac #22828: remove meaningless doctest

comment:12 in reply to: ↑ 11 Changed 5 years ago by
comment:13 followup: ↓ 14 Changed 5 years ago by
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 5 years ago by
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 globalSageMathpointofview).
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 followup: ↓ 16 Changed 5 years ago by
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 5 years ago by
Replying to chapoton:
looks ok, but you should not do that:
from __future__ import absolute_importplease 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 5 years ago by
 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 followup: ↓ 19 Changed 5 years ago by
 Status changed from positive_review to needs_work
PDF docs don't build
comment:19 in reply to: ↑ 18 ; followup: ↓ 21 Changed 5 years ago by
 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 5 years ago by
 Commit changed from 12eb632931b23787b8dc5597e05ff5bfc91aed0c to 69ac798f10445e8c30db25707531ef61a0aaba97
comment:21 in reply to: ↑ 19 Changed 5 years ago by
 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:23 Changed 5 years ago by
 Branch changed from u/dkrenn/22828 to 69ac798f10445e8c30db25707531ef61a0aaba97
 Resolution set to fixed
 Status changed from positive_review to closed
work in progress
New commits:
rich comparison for asymptotic term monoid