Skip to content
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.

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

schemes/elliptic_curves: disable a test that takes too long #36195

Merged
merged 1 commit into from
Sep 10, 2023

Conversation

tornaria
Copy link
Contributor

@tornaria tornaria commented Sep 5, 2023

A doctest introduced in #35626 takes ~50s on current hardware (for comparision, doctesting the whole of src/sage/schemes/elliptic_curves/ell_rational_field.py takes ~22s for 889 tests).

This is too long even for --long, but the example is still interesting, so we just mark it # not tested.

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.

CC: @chriswuthrich (should be a trivial review 🙏)

@chriswuthrich
Copy link
Contributor

Actually, when I implemented this I checked the timings and it wasn't much. If I do ellrank(e) in gp now, it takes 200ms. I believe this could be a bug in the interface with pari rather than a problem with the speed. If so, the test should stay in. I don't know if there were some changes to pari that affect this.

@github-actions
Copy link

github-actions bot commented Sep 5, 2023

Documentation preview for this PR (built with commit 902d91c; changes) is ready! 🎉

@tornaria
Copy link
Contributor Author

tornaria commented Sep 5, 2023

Actually, when I implemented this I checked the timings and it wasn't much. If I do ellrank(e) in gp now, it takes 200ms. I believe this could be a bug in the interface with pari rather than a problem with the speed. If so, the test should stay in. I don't know if there were some changes to pari that affect this.

ellrank is quick; what takes time is saturation. Unfortunately E.rank() also calls (unnecessarily?) saturation.

@chriswuthrich
Copy link
Contributor

Yes, it is the saturation. "ellrank" documentation in pari says "... and L is a list of independent, non-torsion rational points on the curve". I don't think it is unnecessary to saturate them.

I am still puzzled as this test used to be quick. I can't see anything that changed to the saturation function directly.

@tornaria
Copy link
Contributor Author

tornaria commented Sep 5, 2023

For generators it seems necessary. For rank, it seems not.

@chriswuthrich
Copy link
Contributor

Even after playing with old version, I can't see how this ran ever faster. So I think it is totally valid to do what this ticket asks. I must misremember things. Sorry.

The particular test is for the function gens, not rank, so the saturation needs to be done. However I spot that line 2227 in this file can be deleted. I am fine doing this later in a different ticket if this needs to go in quickly. Then I will move that example up to rank so that we have a test involving bigints for pari.

@chriswuthrich
Copy link
Contributor

I am happy for this to go in. Positive (trivial) review. Except I forgot what I need to do in this new interface to signal that.

@vbraun vbraun merged commit 307712d into sagemath:develop Sep 10, 2023
10 checks passed
@tornaria tornaria deleted the ec-doctest branch September 10, 2023 22:45
@mkoeppe mkoeppe added this to the sage-10.2 milestone Sep 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants