Opened 4 months ago
Closed 2 months ago
#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 4 months ago by
- Branch set to u/chapoton/26619
- Commit set to 9eadc3df9642c9b7cc3792aa6078902bd60c468a
- Status changed from new to needs_review
comment:3 Changed 3 months 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 3 months ago by
- Status changed from needs_work to needs_review
should be better now
comment:5 Changed 3 months ago by
- Cc tscrim added
comment:6 Changed 3 months 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 < 210). You are doing that to avoid a doctest failure, right?
comment:7 Changed 3 months 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 3 months 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 3 months ago by
Bot is Green. So is this acceptable or not ?
comment:10 Changed 3 months 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/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 3 months 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/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 3 months 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 3 months ago by
sorry, but I do not understand your last remark. I would be happy enough with the current state.
comment:14 Changed 3 months ago by
I meant changing
-for m in range(1, B+1): +for m in range(1, int(B+1)):
comment:15 Changed 2 months 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 months 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