Opened 6 years ago

Closed 6 years ago

Last modified 4 years ago

#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:

Status badges

Description

as part of #15995 and a step towards python3

Change History (29)

comment:1 Changed 6 years ago by chapoton

  • Branch set to public/21127
  • Cc tscrim jdemeyer dimpase added
  • Commit set to 3f53143b4786213c0f8b1983f0b8f0f80b69f180
  • Status changed from new to needs_review

New commits:

3f53143more cases of python3-compatible divisions

comment:2 Changed 6 years ago by 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.

comment:3 Changed 6 years ago by git

  • Commit changed from 3f53143b4786213c0f8b1983f0b8f0f80b69f180 to 700ec78c1a7e79f8bdef78376883f18060673308

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

700ec78adding future imports of division

comment:4 Changed 6 years ago by chapoton

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 chapoton

ping ?

comment:6 Changed 6 years ago by 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.

comment:7 Changed 6 years ago by 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.

comment:8 Changed 6 years ago by 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?

comment:9 Changed 6 years ago by 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.

comment:10 Changed 6 years ago by 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.

comment:11 Changed 6 years ago by jdemeyer

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

comment:12 Changed 6 years ago by 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.

comment:13 follow-ups: Changed 6 years ago by 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.

comment:14 in reply to: ↑ 13 Changed 6 years ago by 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.

comment:15 in reply to: ↑ 13 Changed 6 years ago by 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.

comment:16 Changed 6 years ago by chapoton

  • Cc darij jmantysalo added

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

comment:17 Changed 6 years ago by chapoton

  • Milestone changed from sage-7.3 to sage-7.4

comment:18 Changed 6 years ago by chapoton

*ping* !

comment:19 Changed 6 years ago by 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.)

comment:20 Changed 6 years ago by 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.

Last edited 6 years ago by jmantysalo (previous) (diff)

comment:21 Changed 6 years ago by git

  • Commit changed from 700ec78c1a7e79f8bdef78376883f18060673308 to 59ed9c40d948d0dc3221671033fa410563116105

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

8353018Merge branch 'public/21127' in 7.4.b0
59ed9c4trac 21127 one detail

comment:22 follow-up: Changed 6 years ago by chapoton

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

comment:23 in reply to: ↑ 22 Changed 6 years ago by 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.

comment:24 follow-up: Changed 6 years ago by chapoton

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 jmantysalo

  • 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 chapoton

  • 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:

272179cmore cases of python3-compatible divisions

comment:27 Changed 6 years ago by jmantysalo

  • Status changed from needs_review to positive_review

OK, I checked the rest.

comment:28 Changed 6 years ago by vbraun

  • 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 embray

  • Commit 272179c1188c531d7a3a5a8aad64002cb4472c5d deleted
  • Keywords division added
Note: See TracTickets for help on using tickets.