#21127 closed enhancement (fixed)
py3: turn some divisions in compatible format
Reported by: | chapoton | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-7.4 |
Component: | python3 | Keywords: | division |
Cc: | tscrim, jdemeyer, dimpase, darij, jmantysalo | Merged in: | |
Authors: | Frédéric Chapoton | Reviewers: | Jori Mäntysalo |
Report Upstream: | N/A | Work issues: | |
Branch: | 272179c (Commits, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description
as part of #15995 and a step towards python3
Change History (29)
comment:1 Changed 6 years ago by
- Branch set to public/21127
- Cc tscrim jdemeyer dimpase added
- Commit set to 3f53143b4786213c0f8b1983f0b8f0f80b69f180
- Status changed from new to needs_review
comment:2 Changed 6 years ago by
It would be nice if you would add from __future__ import division
to those modules.
This might actually fix the doctest in delsarte_bounds.py
with check=False
.
comment:3 Changed 6 years ago by
- Commit changed from 3f53143b4786213c0f8b1983f0b8f0f80b69f180 to 700ec78c1a7e79f8bdef78376883f18060673308
Branch pushed to git repo; I updated commit sha1. New commits:
700ec78 | adding future imports of division
|
comment:4 Changed 6 years ago by
I added import except in the 2 files in misc, because that was changing the behavior and causing doctest failures.
comment:5 Changed 6 years ago by
ping ?
comment:6 Changed 6 years ago by
This is not very fun to review because it is not something which can be done "mechanically". I actually have to read and understand the code to check whether the change is good. And when I see 15 files changed like this, it seems like a lot of work.
comment:7 Changed 6 years ago by
yes, I agree that this is not an easy review. But at least 4 of the files are
just modified by adding optional - python2
.
And two other ones are just adding ...divide
so that the doctest can contain either divide
or true_divide
In the coding file, we just get a correct result instead of an incorrect (but doctested) one.
In the two combinat files, the divisions are exact by congruence imposed on the parameters.
For the graph one, we divide by two an even quantity
For the logic one, we divide by 2 to perform a bit shift
for the knots one, I am not very sure of my change, but the doctests pass.
The crypto and hyperbolic are probably the more subtle ones.
comment:8 Changed 6 years ago by
How did you come up with this patch actually? Your branch seems to contain a random selection of files within Sage. Probably there are a lot of files which needs divisions fixed, so why this selection of files to fix?
comment:9 Changed 6 years ago by
See the bottom of the description of #15995:
export PYTHONIOENCODING="utf-8" source ./local/bin/sage-env python -Qnew ./local/bin/sage-runtests --all
This runs all the tests with the new division behaviour. I have corrected here most of the failing doctests. There remains a few more difficult cases, where I have not been able to locate the problem.
comment:10 Changed 6 years ago by
I think we need a better strategy to deal with divisions.
Ideally, I would like to have a record of every single division operation in Sage. Then, for every division in the Sage sources:
- If that division sometimes has a Sage
Element
as one of the arguments, it should be replaced by a true division. - If that division is always an
int
/int
division, it should be replaced by a floor division.
I think this would take care of most divisions correctly.
comment:11 Changed 6 years ago by
Python actually has a script fixdiv.py which more-or-less does this.
comment:12 Changed 6 years ago by
Hmm.. the -Qxxx
command-line options only affect warnings for Python standard types. There is no way to get a warning for every division, regardless of type.
comment:13 follow-ups: ↓ 14 ↓ 15 Changed 6 years ago by
The need to find a better strategy should not prevent this ticket to be validated.
Anyway, I think our main problem now is cmp
and all its friends.
comment:14 in reply to: ↑ 13 Changed 6 years ago by
Replying to chapoton:
Anyway, I think our main problem now is
cmp
and all its friends.
Hmm, it depends how you look at it. I think that divisions are still harder, mainly because decisions need to be made on a case-by-case basis. For __cmp__
, things are simple: we need to fix everything and we can do that in a pretty uniform way. It is surely a lot of work but at least relatively easy work.
comment:15 in reply to: ↑ 13 Changed 6 years ago by
Replying to chapoton:
The need to find a better strategy should not prevent this ticket to be validated.
Ideally, that better strategy would make this ticket obsolete.
comment:16 Changed 6 years ago by
- Cc darij jmantysalo added
I would nevertheless like to have this ticket reviewed. Any volunteer ?
comment:17 Changed 6 years ago by
- Milestone changed from sage-7.3 to sage-7.4
comment:18 Changed 6 years ago by
*ping* !
comment:19 Changed 6 years ago by
I guess I can look these.
(At the same time this is a test message. It seems that "Active tickets I participated in" do not contain ticket I have been CC'ed any more.)
comment:20 Changed 6 years ago by
You did
- s = RR(1/(RR(n).sqrt() * log(n, 2)**2) * q) + s = q / (RR(n).sqrt() * log(n, 2)**2)
and now if we have, say, n=42
, we got numerical 9.43000415102671
from current version and symbolic 274.197052882637*log(2)^2/log(42)^2
from the new version. This should be asked before the change.
E: I don't think I can be sure about changes to src/sage/crypto/lwe.py
and hope someone else will review this one.
comment:21 Changed 6 years ago by
- Commit changed from 700ec78c1a7e79f8bdef78376883f18060673308 to 59ed9c40d948d0dc3221671033fa410563116105
comment:22 follow-up: ↓ 23 Changed 6 years ago by
I have taken your remark into account. Do you see something else ?
comment:23 in reply to: ↑ 22 Changed 6 years ago by
Replying to chapoton:
I have taken your remark into account. Do you see something else ?
Hmm... There was
sk = ceil((C*(n1+n2))**(3/2))
and in python2 for example 100**(3/2)
is 100
, whereas in python3 it is 1000.0
. But this seems to be a clear bug in current implementation. Maybe your change actually corrects this!
But I can not review this, as I do not know what lwe.py
should do.
comment:24 follow-up: ↓ 25 Changed 6 years ago by
I do not understand what lwe should do either.
Would you give a positive rev. if I revert the changes to lwe ?
comment:25 in reply to: ↑ 24 Changed 6 years ago by
- Reviewers set to Jori Mäntysalo
Replying to chapoton:
Would you give a positive rev. if I revert the changes to lwe ?
I guess so, but haven't read every line yet.
comment:26 Changed 6 years ago by
- Branch changed from public/21127 to u/chapoton/21127
- Commit changed from 59ed9c40d948d0dc3221671033fa410563116105 to 272179c1188c531d7a3a5a8aad64002cb4472c5d
here is a clean branch with no changes to the file crypto/lwe.py
New commits:
272179c | more cases of python3-compatible divisions
|
comment:27 Changed 6 years ago by
- Status changed from needs_review to positive_review
OK, I checked the rest.
comment:28 Changed 6 years ago by
- Branch changed from u/chapoton/21127 to 272179c1188c531d7a3a5a8aad64002cb4472c5d
- Resolution set to fixed
- Status changed from positive_review to closed
comment:29 Changed 4 years ago by
- Commit 272179c1188c531d7a3a5a8aad64002cb4472c5d deleted
- Keywords division added
New commits:
more cases of python3-compatible divisions