Skip to content

undo ancient hack to resolve #35017 #35018

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Feb 8, 2023
Merged

undo ancient hack to resolve #35017 #35018

merged 1 commit into from
Feb 8, 2023

Conversation

yyyyx4
Copy link
Member

@yyyyx4 yyyyx4 commented Feb 8, 2023 •
edited
Loading

The comment in the code says that these coordinates must be integers, but it appears that setting them to base-ring elements works now. This fixes the underlying cause of the random failure. (Resolves #35017.)

@yyyyx4 yyyyx4 marked this pull request as ready for review February 8, 2023 06:11
@yyyyx4 yyyyx4 added this to the sage-9.8 milestone Feb 8, 2023
@kwankyu
Copy link
Collaborator

kwankyu commented Feb 8, 2023

I tried but couldn't catch the bug that really causes the failure. Can you explain?

@yyyyx4
Copy link
Member Author

yyyyx4 commented Feb 8, 2023 •
edited
Loading

What happens here is that sometimes elliptic-curve points are constructed which are "officially" defined over (say) a finite field, but the coordinates are represented as integers. This breaks some assumptions I made writing code like this: The .universe() will be ZZ or even int, which (in this particular case) causes the .base_extend() call to try base-changing the curve to ZZ. That's wrong, and usually impossible. The patch makes sure that the coordinates of the point lie in the alleged base ring.

@kwankyu
Copy link
Collaborator

kwankyu commented Feb 8, 2023

Isn't it int in this case, since the failure is in sage.rings.finite_rings.element_givaro.FiniteField_givaroElement.__int__?

@kwankyu
Copy link
Collaborator

kwankyu commented Feb 8, 2023

Here in the traceback:

      File "/Users/kwankyu/GitHub/sage-dev/src/sage/schemes/elliptic_curves/ell_generic.py", line 1355, in <listcomp>
        E = constructor.EllipticCurve([R(a) for a in self.a_invariants()])

R is ZZ?

@kwankyu
Copy link
Collaborator

kwankyu commented Feb 8, 2023

Okay. Now I understand the situation. Thanks.

Let's wait for the checks for sanity.

@JohnCremona JohnCremona self-requested a review February 8, 2023 08:44
@codecov-commenter
Copy link

Codecov Report

Base: 88.59% // Head: 88.59% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (64cfac5) compared to base (6a4667b).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files 
@@           Coverage Diff            @@
##           develop   #35018   +/-   ##
========================================
  Coverage    88.59%   88.59%           
========================================
  Files         2136     2136           
  Lines       396141   396142    +1     
========================================
+ Hits        350977   350979    +2     
+ Misses       45164    45163    -1
Impacted Files Coverage Δ
src/sage/schemes/elliptic_curves/ell_point.py 84.63% <100.00%> (+0.01%) ⬆️
src/sage/groups/affine_gps/euclidean_group.py 88.88% <0.00%> (-11.12%) ⬇️
src/sage/categories/super_modules_with_basis.py 96.29% <0.00%> (-3.71%) ⬇️
src/sage/groups/affine_gps/affine_group.py 96.62% <0.00%> (-2.25%) ⬇️
...rc/sage/categories/super_lie_conformal_algebras.py 95.00% <0.00%> (-1.67%) ⬇️
...onformal_algebras/lie_conformal_algebra_element.py 95.52% <0.00%> (-1.50%) ⬇️
src/sage/doctest/forker.py 80.24% <0.00%> (-1.48%) ⬇️
src/sage/modular/hecke/algebra.py 94.65% <0.00%> (-1.07%) ⬇️
src/sage/combinat/posets/poset_examples.py 87.67% <0.00%> (-1.01%) ⬇️
src/sage/homology/matrix_utils.py 87.15% <0.00%> (-0.92%) ⬇️
... and 21 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@yyyyx4 yyyyx4 requested a review from kwankyu February 8, 2023 09:16
kwankyu
kwankyu approved these changes Feb 8, 2023
View reviewed changes
Copy link
Collaborator

@kwankyu kwankyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@dimpase dimpase merged commit 17d8a73 into sagemath:develop Feb 8, 2023
@yyyyx4 yyyyx4 deleted the public/35017 branch February 8, 2023 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kwankyu kwankyu kwankyu approved these changes

+1 more reviewer

@JohnCremona JohnCremona JohnCremona approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
c: elliptic curves p: blocker / 1 t: bug t: tests
Projects
None yet
Milestone
sage-9.8
Development

Successfully merging this pull request may close these issues.

random doctest failure in src/sage/schemes/elliptic_curves/ell_curve_isogeny.py
6 participants
@yyyyx4 @kwankyu @codecov-commenter @JohnCremona @dimpase @mkoeppe