#26619 closed enhancement (fixed)

py3: one fix in bdd_height.py

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.5
Component: python3 Keywords:
Cc: tscrim Merged in:
Authors: Frédéric Chapoton Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 5b1085b (Commits) Commit: 5b1085b40df9179480ed66278393f9d084e190d5
Dependencies: Stopgaps:

Description

plus some pep cleanup

Change History (16)

comment:1 Changed 13 months ago by chapoton

  • Branch set to u/chapoton/26619
  • Commit set to 9eadc3df9642c9b7cc3792aa6078902bd60c468a
  • Status changed from new to needs_review

New commits:

9eadc3dpy3: one fix in bdd_height

comment:2 Changed 13 months ago by chapoton

  • Status changed from needs_review to needs_work

failing doctest

comment:3 Changed 13 months ago by git

  • Commit changed from 9eadc3df9642c9b7cc3792aa6078902bd60c468a to 5c3ef67840c4d65e226fcd36d83f1d550df5ad4d

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

5c3ef67trac 26619 using ceil instead of floor : better

comment:4 Changed 13 months ago by chapoton

  • Status changed from needs_work to needs_review

should be better now

comment:5 Changed 13 months ago by chapoton

  • Cc tscrim added

comment:6 Changed 13 months ago by tscrim

if RR(n*y).ceil() is n*y: # WHAT !?

+1

Although I am not convinced that

-        sage: enum_projective_number_field(X(K), bound=5^(1/3), prec=2^10)
+        sage: enum_projective_number_field(X(K), bound=RR(5^(1/3)), prec=2^10)

should be changed. It seems like you are loosing precision on bound by doing that (since 53 < 210). You are doing that to avoid a doctest failure, right?

comment:7 Changed 13 months ago by chapoton

I am doing that because of the "round" problem.. only hiding under the carpet the issue that "float" has no method "round"..

comment:8 Changed 12 months ago by git

  • Commit changed from 5c3ef67840c4d65e226fcd36d83f1d550df5ad4d to 5b1085b40df9179480ed66278393f9d084e190d5

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

5b1085btrac 26619 pyflakes cleanup

comment:9 Changed 12 months ago by chapoton

Bot is Green. So is this acceptable or not ?

comment:10 Changed 12 months ago by tscrim

  • Reviewers set to Travis Scrimshaw

This is acceptable, but I want to see if we can quickly fix the underlying issue. Can you post the full traceback of the failure without your change? From looking at the code, I do not see where a round is called, but maybe I am missing something.

I am also seeing another possible py3 fix needed:

/home/travis/sage-build/local/lib/python2.7/site-packages/sage/rings/number_field/bdd_height.pyc in bdd_height(K, height_bound, tolerance, precision)
    535     possible_norm_set = set([])
    536     for n in range(class_number):
--> 537         for m in range(1, B+1):
    538             possible_norm_set.add(m*class_group_rep_norms[n])
    539     bdd_ideals = bdd_norm_pr_ideal_gens(K, possible_norm_set)

where B+1 needs to be cast to an int here.

comment:11 Changed 12 months ago by chapoton

The best fix would be to add __round___ to our own real numbers.. See #25827 for the reasons why this seems to be difficult and nobody did it.

Here is a python3 traceback involving one of these range(1, B+1):

File "src/sage/schemes/projective/projective_space.py", line 1339, in sage.schemes.projective.projective_space.ProjectiveSpace_field.points_of_bounded_height
Failed example:
    len(list(P.points_of_bounded_height(bound=1.5, tolerance=0.1)))
Exception raised:
    Traceback (most recent call last):
      File "/home/u1/chapoton/sage3/local/lib/python3.6/site-packages/sage/doctest/forker.py", line 671, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/u1/chapoton/sage3/local/lib/python3.6/site-packages/sage/doctest/forker.py", line 1086, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.schemes.projective.projective_space.ProjectiveSpace_field.points_of_bounded_height[4]>", line 1, in <module>
        len(list(P.points_of_bounded_height(bound=RealNumber('1.5'), tolerance=RealNumber('0.1'))))
      File "/home/u1/chapoton/sage3/local/lib/python3.6/site-packages/sage/schemes/projective/projective_space.py", line 1363, in points_of_bounded_height
        for x in iters: next(x) # put at zero
      File "/home/u1/chapoton/sage3/local/lib/python3.6/site-packages/sage/rings/number_field/bdd_height.py", line 537, in bdd_height
        for m in range(1, B+1):
    TypeError: 'sage.rings.real_mpfr.RealNumber' object cannot be interpreted as an integer

comment:12 Changed 12 months ago by tscrim

Thank you.

Do we just want to cast int(B+1)? If you just want to keep things as they are, then you can set a positive review.

comment:13 Changed 12 months ago by chapoton

sorry, but I do not understand your last remark. I would be happy enough with the current state.

comment:14 Changed 12 months ago by tscrim

I meant changing

-for m in range(1, B+1):
+for m in range(1, int(B+1)):

comment:15 Changed 12 months ago by chapoton

  • Status changed from needs_review to positive_review

oh, well. We will see later. Let us move on here. I am setting to positive..

Last edited 12 months ago by chapoton (previous) (diff)

comment:16 Changed 12 months ago by vbraun

  • Branch changed from u/chapoton/26619 to 5b1085b40df9179480ed66278393f9d084e190d5
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.