allow non-minimal equation for padic heights #35614
Conversation
|
I don't understand the error in pycodestyle. Line 332 in constructor.py is |
You just need to start any doctest containing latex macros with |
Oh, of course, thanks a lot for the help, Frédéric. I am a bit rusty. |
|
This is ready for review in component "elliptic curves". (Can I add labels myself?) |
|
I will review this |
| li = [] | ||
| for p in ps: | ||
| np = u.valuation(p) | ||
| if Emin.discriminant() %p != 0: |
There was a problem hiding this comment.
I prefer keeping it. Old habit to think like a mathematician: I think they increase the readability of the code, but don't slow it down significantly. But you insist, I am ok taking it off.
There was a problem hiding this comment.
I don't insist (I do the same) but some python purists might
| else: # non split | ||
| li.append(E.tamagawa_number(p) * (p+1) * p**(np-1)) | ||
| otherbad = Integer(Emin.discriminant()).prime_divisors() | ||
| otherbad = [p for p in otherbad if u%p != 0 ] |
|
The added commits contain the requested doctests in padics.py. And it corrects my earlier changes of "√élu". It now reads "square-root Vélu" instead. A discussion on this issue took place in the pull request #35626 . I am open for a larger discussion of this question and change it back to "√élu". |
|
I have clicked on "resolved" on the three reviewer's comments above that I think I have resolved. I don't know if that is my job or the reviewers' job. |
|
Probably my job to mark them as resolved, I will look again soon |
|
Merge conflict |
|
I'll switch this back to +ve review when tests pass |
|
Sorry wrong ticket! |
|
Documentation preview for this PR (built with commit 02ee0e2) is ready! 🎉 |
gh-35626: Update the rank function of elliptic curves to use ellrank in pari ### 📚 Description Fixes #35621 Pari has now a very good in-built algorithm to determine the rank and a set of generators of the Mordell-Weil group of elliptic curves over the rational. It is, as far as I understand, a port of Denis Simon's script ellQ.gp. It performs a 2-descent and uses the Cassels Tate pairing to conclude the rank in many cases - when Sha has no 4-torsion elements. The functionality to search rational points is also very quick. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x ]`. --> - `[x]` The title is concise, informative, and self-explanatory. - `[x]` The description explains in detail what this PR is about. - `[x]` I have linked a relevant issue or discussion. - `[x]` I have created tests covering the changes. - `[x]` I have updated the documentation accordingly. ### ⌛ Dependencies - #35614 a pull request that fixes, among other things, the documentation in the files changed here. To avoid problems, I based it on that PR. <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: #35626 Reported by: Chris Wuthrich Reviewer(s): Chris Wuthrich, John Cremona, Lorenz Panny
📚 Description
Fixes #34790
The
p-adic height calculations on an elliptic curve assumed that the equation was minimal.This is fixed as well as some docstring formatting.
The main mathematical change is in padics.py, new line 579, which is then used in new line 931. This makes sure that the point is multiplied by an integer such that it has good reduction at all primes, even if the model is not minimal. Though it is still assumed that the models is integral.
The rest is cosmetics.
📝 Checklist
[x]The title is concise, informative, and self-explanatory.[x]The description explains in detail what this PR is about.[x]I have linked a relevant issue or discussion.[x]I have created tests covering the changes.[x]I have updated the documentation accordingly.(My first pull request, tell me if I am doing something wrong or where I need to read up on how we do things now.)