py3: turn some divisions in compatible format

[chapoton]

as part of #15995 and a step towards python3

[chapoton]

New commits:

​3f53143

more cases of python3-compatible divisions

[jdemeyer]

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.

[git]

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

​700ec78

adding future imports of division

[chapoton]

I added import except in the 2 files in misc, because that was changing the behavior and causing doctest failures.

[chapoton]

ping ?

[jdemeyer]

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.

[chapoton]

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

[jdemeyer]

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?

[chapoton]

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.

[jdemeyer]

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:

1. If that division sometimes has a Sage Element as one of the arguments, it should be replaced by a true division.
2. 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.

[jdemeyer]

Python actually has a script ​fixdiv.py which more-or-less does this.

[jdemeyer]

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.

[chapoton]

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.

[jdemeyer]

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.

[jdemeyer]

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.

[chapoton]

I would nevertheless like to have this ticket reviewed. Any volunteer ?

[chapoton]

*ping* !

[jmantysalo]

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

[jmantysalo]

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.

[git]

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

​8353018	Merge branch 'public/21127' in 7.4.b0
​59ed9c4	trac 21127 one detail

[chapoton]

I have taken your remark into account. Do you see something else ?

[jmantysalo]

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.

[chapoton]

I do not understand what lwe should do either.

Would you give a positive rev. if I revert the changes to lwe ?

[jmantysalo]

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.

[chapoton]

here is a clean branch with no changes to the file crypto/lwe.py

---

New commits:

​272179c

more cases of python3-compatible divisions

[jmantysalo]

OK, I checked the rest.