Opened 5 years ago
Closed 5 years ago
#22828 closed enhancement (fixed)
py3: rich comparison for asymptotic term monoid
Reported by:  Frédéric Chapoton  Owned by:  

Priority:  major  Milestone:  sage8.0 
Component:  python3  Keywords:  
Cc:  Clemens Heuberger, Benjamin Hackl, Daniel Krenn  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:  → u/chapoton/22828 

Commit:  → 0e790b08aab3898c846faab3f29f6ce45b83df71 
comment:2 Changed 5 years ago by
Commit:  0e790b08aab3898c846faab3f29f6ce45b83df71 → 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:  7497d362f39547dff48c526b17561c2836dad761 → 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:  4c8c3180e1e5ab42c3d2df1a188bc14234816339 → 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:  Clemens Heuberger Benjamin Hackl Daniel Krenn added 

Description:  modified (diff) 
comment:6 Changed 5 years ago by
Commit:  efd771d1245275d6a02df85d6681e8063dfc2e8e → 366066890786c51cacdfda64edafe82bb2bd9e82 

Branch pushed to git repo; I updated commit sha1. New commits:
3660668  trac 22828 fix imports

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:  u/chapoton/22828 → u/dkrenn/22828 

comment:10 Changed 5 years ago by
Authors:  Frédéric Chapoton → Frédéric Chapoton, Daniel Krenn 

Commit:  366066890786c51cacdfda64edafe82bb2bd9e82 → 9b2a790c5cb6ce7ef776056ecba0f31af62ad1bf 
Reviewers:  → Daniel Krenn 
Status:  new → 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:  9b2a790c5cb6ce7ef776056ecba0f31af62ad1bf → 12eb632931b23787b8dc5597e05ff5bfc91aed0c 

Branch pushed to git repo; I updated commit sha1. New commits:
12eb632  Trac #22828: remove meaningless doctest

comment:12 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 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 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:  Daniel Krenn → Daniel Krenn, Frédéric Chapoton 

Status:  needs_review → positive_review 
ok, indeed. sorry for the noise.
Then let it be.
comment:18 followup: 19 Changed 5 years ago by
Status:  positive_review → needs_work 

PDF docs don't build
comment:19 followup: 21 Changed 5 years ago by
Status:  needs_work → 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:  12eb632931b23787b8dc5597e05ff5bfc91aed0c → 69ac798f10445e8c30db25707531ef61a0aaba97 

comment:21 Changed 5 years ago by
Status:  needs_info → 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:  u/dkrenn/22828 → 69ac798f10445e8c30db25707531ef61a0aaba97 

Resolution:  → fixed 
Status:  positive_review → closed 
work in progress
New commits:
rich comparison for asymptotic term monoid