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: |
Description (last modified by )
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
- Branch set to u/chapoton/22828
- Commit set to 0e790b08aab3898c846faab3f29f6ce45b83df71
comment:2 Changed 4 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 4 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 4 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 4 years ago by
- Cc cheuberg behackl dkrenn added
- Description modified (diff)
comment:6 Changed 4 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 4 years ago by
I'll have a look at the changes in the next days.
comment:8 Changed 4 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 4 years ago by
- Branch changed from u/chapoton/22828 to u/dkrenn/22828
comment:10 Changed 4 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 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
|
comment:11 follow-up: ↓ 12 Changed 4 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 4 years ago by
comment:13 follow-up: ↓ 14 Changed 4 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 4 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 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: ↓ 16 Changed 4 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 4 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 4 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 follow-up: ↓ 19 Changed 4 years ago by
- Status changed from positive_review to needs_work
PDF docs don't build
comment:19 in reply to: ↑ 18 ; follow-up: ↓ 21 Changed 4 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 4 years ago by
- Commit changed from 12eb632931b23787b8dc5597e05ff5bfc91aed0c to 69ac798f10445e8c30db25707531ef61a0aaba97
comment:21 in reply to: ↑ 19 Changed 4 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 4 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