Opened 2 years ago
Closed 2 years ago
#26619 closed enhancement (fixed)
py3: one fix in bdd_height.py
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage8.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 2 years ago by
 Branch set to u/chapoton/26619
 Commit set to 9eadc3df9642c9b7cc3792aa6078902bd60c468a
 Status changed from new to needs_review
comment:3 Changed 2 years ago by
 Commit changed from 9eadc3df9642c9b7cc3792aa6078902bd60c468a to 5c3ef67840c4d65e226fcd36d83f1d550df5ad4d
Branch pushed to git repo; I updated commit sha1. New commits:
5c3ef67  trac 26619 using ceil instead of floor : better

comment:4 Changed 2 years ago by
 Status changed from needs_work to needs_review
should be better now
comment:5 Changed 2 years ago by
 Cc tscrim added
comment:6 Changed 2 years ago by
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 < 2^{10}). You are doing that to avoid a doctest failure, right?
comment:7 Changed 2 years ago by
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 2 years ago by
 Commit changed from 5c3ef67840c4d65e226fcd36d83f1d550df5ad4d to 5b1085b40df9179480ed66278393f9d084e190d5
Branch pushed to git repo; I updated commit sha1. New commits:
5b1085b  trac 26619 pyflakes cleanup

comment:9 Changed 2 years ago by
Bot is Green. So is this acceptable or not ?
comment:10 Changed 2 years ago by
 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/sagebuild/local/lib/python2.7/sitepackages/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 2 years ago by
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/sitepackages/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/sitepackages/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/sitepackages/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/sitepackages/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 2 years ago by
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 2 years ago by
sorry, but I do not understand your last remark. I would be happy enough with the current state.
comment:14 Changed 2 years ago by
I meant changing
for m in range(1, B+1): +for m in range(1, int(B+1)):
comment:15 Changed 2 years ago by
 Status changed from needs_review to positive_review
oh, well. We will see later. Let us move on here. I am setting to positive..
comment:16 Changed 2 years ago by
 Branch changed from u/chapoton/26619 to 5b1085b40df9179480ed66278393f9d084e190d5
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
py3: one fix in bdd_height