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

Better and more consistent display of invariants and generators for ECQ and ECNF #6279

Merged
merged 3 commits into from
Dec 2, 2024

Conversation

JohnCremona
Copy link
Member

This deals with the points raised in #6275 and #6276

@JohnCremona
Copy link
Member Author

I am finding and fixing some test failures now...

@AndrewVSutherland
Copy link
Member

How are you ordering generators? It looks like infinite order generators come first, which seems fine, but among the infinite order generators it looks like on ECQ pages they are typically ordered by height (at least in the examples I looked at), but this is not true on the ECNF pages (see http://127.0.0.1:37777/EllipticCurve/2.0.3.1/44563.2/a/1 for example). I think it would be better to always order them by height.

I could see an argument for ordering all the points by height (which would put the torsion points first, in which case we would probably want to change the way the MW group structure is written to match), but I don't feel strongly about this.

@JohnCremona
Copy link
Member Author

Over Q the generators of infinite order are indeed always in height order. In detail, from any set of generators (mod torsion) I do a Minkowski reduction to get a new set, so the first generator has minimal height, etc. After that, I try adding all torsion points to each of these minimal generators (recall tat this does not change the canonical height), picking whichever is smallest in a naive sense (literally, the shortest string representation). The code for this is in https://github.com/JohnCremona/ecdata/blob/master/scripts/red_gens.py. About 4 years ago I ran this on the elliptic curves over Q and changed the stored generators in the database. So the situation over Q is good. (I should say that I did all this after prompting by Randall Rathbun, who had written his own code for "reducing" the generators in much the same way, but in the end I wrote my own.)

I could easily carry out the same process for ECNF, but have not done so. Let's make a new issue for that. It should not take ong as the code for handling generators over Q will work with trivial changes (if any), and there are fewer curves.

After this, the non-torsion generators over number fields would be ordered by height and minimal in the above sense, which is what we surely want. I personally would not want to move the torsion generators first, even if they have height 0. Instead, I had thought of having the torsion and non-torsion generators separated in the table, or in two tables, so the non-torsion ones would not need to have \infty in the order column and the torsion ones would not need to have 0 in the height column. But I would also be happy to leave the table as it is (after improving the generators for ECNF).

Anyway, the proposed improvement of ECNF generators does not require any more changes to code or templates, so can we merge this now?

@AndrewVSutherland
Copy link
Member

Another option would be to just sort the generators when they are displayed on the page (rather than updating the data), but if you prefer to do that in the data, that's fine. I don't think we need two tables and think that it's fine to list the infinite order generators first (but it would be nice to have these in height order).

@AndrewVSutherland AndrewVSutherland merged commit 35e5db3 into LMFDB:main Dec 2, 2024
13 checks passed
@JohnCremona
Copy link
Member Author

OK, I'll add a sort now (and make a new PR) while I work on improving the data.

@JohnCremona JohnCremona deleted the ECgens branch December 3, 2024 08:40
@JohnCremona
Copy link
Member Author

OK, I'll add a sort now (and make a new PR) while I work on improving the data.

This is #6284 -- sorry not to have just added to this PR, as it was only a one-line addition.

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.

2 participants