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
modify the NumberField constructor to pass in optional integer B such that all the internal pari routines will replace the discriminant by its gcd with B, making some things massively faster. #9400
Comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
comment:3
Example of what this makes possible:
|
comment:5
It is clear from a very first look at the patch that this will massively conflict with #9343 (why did nobody point this out to me earlier???). Personally, I would prefer to postpone the last two points of the description, i.e.
and do them after #9343 is merged (I also want to change the hashing of PARI objects, see #9667). Also, I'm personally not completely convinced about the best way to print NumberFieldIdeals (see also my post at sage-devel). |
comment:6
I'm sure this will be easy to merge with #9343. It's probably best to get 9343 in first, since it is much more difficult, then rebase the current patch against it.
The current hashing and printing setup is complete and total crap, and needs to be removed ASAP. It renders number fields completely useless for any nontrivial applications that involve prime ideals.
I saw that. I think the best solution is exactly what I implemented in the attached patch. Hash based on gens (trivial, as a hash should be). Print in reduced form only in small trivial cases (by default), but allow the user to easily up the cutoff if they want, for some reason. |
comment:7
Replying to @williamstein:
Why not hash based on the HNF representation, as I propsed in #9666? I think this is more canonical than gens() and it is the native PARI format. |
comment:9
Replying to @jdemeyer:
CONS: PROS: Thus taken together, I'm OK with your proposal with the caveat that at some future time it has to be revisited for relative extensions. |
apply only this patch (which also addresses the referees issue with |
comment:10
Attachment: trac_9400.patch.gz OK, new patch up that changes hash. It passes all doctests on sage.math. |
comment:11
The patch introduces an inconsistency:
Note how the integer is printed first in the first case but last in the second case (and personally I find it clearer when the integer is put first). Maybe the sorting and uniqueing should be done in the NumberFieldIdeal constructor instead of when the ideal is printed? |
comment:12
Would that make any difference? The difference above is that in one case the number field has a very small discriminant (-1323), and in the other it doesn't (-2700540027). When the discriminant is small, then reduced generators are used for printing. A solution could be to apply the sorting and "uniquing" in both cases before printing -- right now it is only applied in the case of large discriminant. |
comment:13
Hi, I retract my comment. The issue may be that sorting of elements of number fields is now useless. Observe:
Thus it doesn't matter what you do with sorting and uniquing the gens before or after -- there's no sensible ordering that will come out of this, unless elements of number fields get a total (non-algebraic) ordering. I thought they had one. Oh, now I remember -- there is a major bug in the way elements of number fields are ordered. You can see this by looking at the code (I think Joel Mohler) wrote in number_field_element.pyx. I fixed this a few weeks ago as part of the patch bomb #9541. So my advice is to not worry about sorting issues as part of this patch, but keep in mind that it is has to be fixed later. I've opened ticket #9752 to fix this. |
comment:14
Always using
In my opinion, the best way would be to use |
comment:15
|
comment:16
I am not familiar with |
comment:17
Rebasing to #9343 will be easier if we seperate the "maximize_at_primes part" from the "printing and hashing of ideals" part. So I will cut this patch in two pieces. |
Patch for the "maximize at primes" part, rebased to sage-4.6.prealpha1 (see #9343) |
comment:18
Attachment: 9400_maximize_at_primes.patch.gz |
This comment has been minimized.
This comment has been minimized.
comment:19
I attached a big reviewer patch (to be applied on top of 9400_maximize_at_primes.patch) changing:
Since the purpose of |
Apply on top of 9400_maximize_at_primes.patc |
comment:20
Attachment: 9400_jd_review.patch.gz |
This comment has been minimized.
This comment has been minimized.
Attachment: 9400_docfix.patch.gz Apply on top of previous 2 patches, small documentation fixes |
Changed reviewer from Jeroen Demeyer, William Stein, John Cremona to Jeroen Demeyer, John Cremona |
Attachment: 9400_combined.patch.gz All 3 patches combined (apply only this patch) |
This comment has been minimized.
This comment has been minimized.
comment:27
I did all the changes requested by the reviewer. Replying to @JohnCremona:
This belongs to #9636 and is fixed there. The other warnings are fixed here. |
comment:28
Hi, I tried to apply my patch to 4.5.2.rc0 and it works fine:
I tried to apply your patch (9400_combined.patch) and there are a massive number of rejects:
|
This comment has been minimized.
This comment has been minimized.
comment:29
Replying to @williamstein:
It is meant to be applied after #9343, then it works fine. |
comment:30
John, do Jeroen's most recent changes look good to you? |
comment:31
Replying to @qed777:
Sorry for delay -- Jeroen has nudged me on this and I'll look at it as soon as I can, but I'm at a conference this week... |
comment:32
Replying to @qed777:
The new combined patch does look good. It applies smoothly to 4.6.alpha0, and the docs (re)build with no warnings. I am currently doing a full test just to make sure. Will report back shortly. |
comment:33
Replying to @JohnCremona:
All (long) tests pass -- positive review. |
Merged: sage-4.6.alpha1 |
comment:35
Replying to @qed777: Note that I mentioned in this ticket (and on sage-devel) that I do not understand |
Also:
ReSTify residue class fields
massively optimize reduction modulo a prime (e.g., the first interesting example I tried, I got a speedup of a factor of 500,000! Yes half a million times faster!).
Parts of this patch have been moved to #9764.
Apply only
9400_combined.patch
and apply it on top of the patches from #9343. See also http://wiki.sagemath.org/NewPARI.CC: @jdemeyer
Component: number fields
Keywords: PARI number field
Author: William Stein, Jeroen Demeyer
Reviewer: Jeroen Demeyer, John Cremona
Merged: sage-4.6.alpha1
Issue created by migration from https://trac.sagemath.org/ticket/9400
The text was updated successfully, but these errors were encountered: