Opened 9 years ago

Last modified 8 years ago

#9334 closed enhancement

Implement Hilbert symbols over number fields — at Version 25

Reported by: aly.deines Owned by: davidloeffler
Priority: major Milestone: sage-4.8
Component: number fields Keywords: hilbert symbol
Cc: mstreng, jdemeyer Merged in:
Authors: Aly Deines, Marco Streng Reviewers: David Loeffler, John Cremona, Marco Streng
Report Upstream: Fixed upstream, in a later stable release. Work issues: bug in Pari 2.4.3
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by mstreng)

Pari has Hilbert symbols for number fields. Hilbert symbols can be implemented by wrapping pari's function nfhilbert.

Change History (27)

comment:1 Changed 9 years ago by aly.deines

  • Status changed from new to needs_work

Here all the functions are better placed. I still need to fix the code so that generalized_hilbert_symbol(a,b,P) doesn't assume a.valuation(P) and b.valuation(P) are 0 or 1.

comment:2 Changed 9 years ago by aly.deines

  • Status changed from needs_work to needs_review

I changed the code as Tim (correctly) suggested so as it doesn't assume reduced input.

comment:3 Changed 9 years ago by aly.deines

This has better uniformizer code.

comment:4 Changed 9 years ago by davidloeffler

  • Status changed from needs_review to needs_work
  • Work issues set to patch does not apply

This doesn't seem to apply to 4.4.4. Does it require some other patch as a prerequisite? Also, the docstrings don't seem to be correctly ReST formatted (you should always run sage -docbuild reference html and check that there are no warnings before submitting a patch).

comment:5 Changed 9 years ago by davidloeffler

  • Work issues changed from patch does not apply to ReST formatting issues

I see. So it's supposed to be applied on top of the patches at #9317. That's fine, but you should explain this in your trac upload messages. Don't repost random patches from other tickets on this ticket -- that's just unnecessary duplication, and it's confusing for the release maintainer when s/he has to merge stuff later.

Anyway: with the #9317 patches in place these four patches apply fine, and all doctests pass. But they're quite hard to review, since you seem to have added code in one place in the first patch and then removed it and added it again somewhere else in the second. Could I suggest that you use the Mercurial "qfold" command to combine the four patches into one single patch? That would make the reviewer's job vastly easier. And don't forget those docstring formatting problems; the two that stand out most at a quick glance are that the LaTeX formulae should be in backticks not dollar signs (`x^2 + 2` etc), and the LaTeX fraction command is \frac not \frak.

comment:6 Changed 9 years ago by aly.deines

Here is one single patch. It does not depend on ticket #9317. This should have all the documentation fixed. Thank you for your patience, I've learned a lot about Mercurial and ReST formatting recently.

I also applied this on a clean clone of 4.4.2 to check that it would build, all the doctest pass, and the -docbuild looks correct.

comment:7 Changed 9 years ago by davidloeffler

Most of this looks fine, and the docstring formatting is much better; but there are some technical issues.

  • The code in solver_mod_p is obviously wrong for n > 1: it calculates the inverse modulo Pn but then takes the square root of this modulo P. You need some kind of Hensel lifting or suchlike to get an answer that's right modulo Pn.
  • The code in uniformizer is a mess (e.g. it trivially fails for any non-principal ideal in a number field of degree > 2, because you've assumed self.integral_basis() has length 2). But there's already a method sage.rings.number_field.number_field.NumberField_generic.uniformizer (taking a prime as an argument). I agree that it is worth having uniformizers accessible via a method of ideals as well, but it should just be a thin wrapper around the existing code.

comment:8 Changed 9 years ago by cremona

  • Cc mstreng added
  • Milestone changed from sage-wishlist to sage-5.0
  • Reviewers set to David Loeffler, John Cremona
  • Work issues changed from ReST formatting issues to ReST formatting issues, and more

I generally agree with David's points. This code will be very useful for a topic begin done at SD23 (solving conics over various fields) so I am keen to get this in (suitably modified).

In generalized_legendre_symbol: (1) test P for primality first, before trying to construct its residue field. (2) instead of K(2).valuation(P) just test that n is odd. (3) don't raise run-time errors, make them ValueErrors??. (4) make the return types consistent: you return either +1 in k or -1 as a python int. I would return a Sage integer in either case. (5) you do not test if P divides self. If so, return 0 (as a Sage integer)>

Why are generalized_hilbert_symbol and _legendre_symbol in sage/rings/arith.py? I would put them both in number_fields -- where you put the even one in fact.

In generalized_even_hilbert_symbol you define but do not use iprime, so delete it. And do the simple calculation to get the coefficients of jprime2 so you don't need to construct the quaternion algebra. (You can leave in a comment about that).

_voight_alg_6_2 has some symbols which should be . Check for others.

Do what David said about uniformizer -- just call the existing function.

Sort out the solve function.

comment:9 Changed 9 years ago by mstreng

Great, I could use this.

While you are still at it, I have a small wish list as well. Could you

  • Let the generalized even Hilbert symbol accept fractions (as the odd one and the QQ one do)?
    sage: hilbert_symbol(1/3, 1, 2)
    1
    sage: K.<i> = QuadraticField(-1)
    sage: O = K.maximal_order()
    sage: generalized_hilbert_symbol(K(1/2), K(1), 3*O)
    1
    sage: generalized_hilbert_symbol(K(1/3), K(1), (1+i)*O)
    NotImplementedError: inverse_mod is not implemented for non-integral elements
    
  • Also add the Hilbert symbol for infinite places? See e.g.
    sage: hilbert_symbol(-1, -1, -1)
    -1
    
    This is almost trivial compared to what you've already done. I have code, contact me if you have questions.
  • Correct the doc text. The doc of generalized_even_hilbert_symbol should say that P must divide 2, while generalized_hilbert_symbol should not say that P must be odd

comment:10 Changed 9 years ago by mstreng

In addition to the first part of my precious comment: generalized_even_hilbert_symbol should accept a and b to both be divisible by p.

sage: hilbert_symbol(2,2,2)
1
sage: K.<i>=QuadraticField(-1)
sage: O=K.maximal_order()
sage: generalized_hilbert_symbol(3,3,3*O)
1
sage: generalized_hilbert_symbol(2,2,2*O)
ValueError: P must be a prime

comment:11 Changed 9 years ago by mstreng

Oops, the last two lines of my previous comment should of course read

sage: p = 1+i
sage: generalized_hilbert_symbol(p,p,p*O)
RuntimeError: ord_P(a) or ord_P(b) must be zero

comment:12 Changed 9 years ago by mstreng

  • Priority changed from minor to major

comment:13 Changed 9 years ago by cremona

  • Description modified (diff)
  • Summary changed from hilbert symbols!!! to Implement Hilbert symbols over number fields

Alyson, are you intending to fix the various points raised by reviewers here? If not, someone else should. Ticket #9320 is waiting on this one.

comment:14 Changed 9 years ago by aly.deines

Here are the changes I've made so far:

  1. in generalized_legendred_symbol I test for primality first
  2. instead of K(2).valuation(P) I just test that n is odd
  3. I've changed RunTime? Errors to ValueErrors?
  4. I return +/- 1 as sage integers
  5. I test if P|self and if so return 0 (as a sage integer)
  6. in generalized_hilbert_symbol I deleted iprime
  7. I did the calculation and have hard coded jprime2
  8. I've replace with where necessary in _voight_alg_6_2
  9. Things should work for fractions
  10. generalized_hilbert_symbol should also accept a,b, divisible by p

One question I have, does anyone know about hensel lifting in sage?

comment:15 Changed 9 years ago by cremona

Excellent -- should it be "needs review" again then?

There must be places in Sage where Hensel lifting is done, but I do not know of any general framework for it. You could try asking David Roe, who (I think) wrote a lot of the p-adic code.

comment:16 Changed 9 years ago by aly.deines

No, it shouldn't be "needs review" yet. I still need to fix solver_mod_p.

comment:17 Changed 9 years ago by aly.deines

  • Status changed from needs_work to needs_review

I have fixed solver_mod so that it computes the square root mod Pn.

comment:18 Changed 9 years ago by aly.deines

Fixes/added documentation. Has the examples above in the documentation.

comment:19 Changed 9 years ago by rlm

  • Status changed from needs_review to needs_work

Based on 4.5.3.alpha1:

sage -t  sage/rings/number_field/number_field_ideal.py
**********************************************************************
File "/home/rlmill/sage-4.5.3.alpha1/devel/sage-main/sage/rings/number_field/number_field_ideal.py", line 1212:
    sage: P.uniformizer()
Expected:
    a + 4
Got:
    -2*a + 1
**********************************************************************
File "/home/rlmill/sage-4.5.3.alpha1/devel/sage-main/sage/rings/number_field/number_field_ideal.py", line 1219:
    sage: P.uniformizer()
Expected:
    -7*a^4 + 13*a^3 - 13*a^2 - 2*a + 50
Got:
    a^4 - a^3 + a^2 - a + 1
**********************************************************************

Changed 9 years ago by aly.deines

Fixes some documentation.

comment:20 Changed 9 years ago by aly.deines

  • Status changed from needs_work to needs_review

Ok, I checked at P.uniformizer() is just wrapping K.uniformizer(P). So assuming K.uniformizer(P) is correct (which it appears to be), I've fixed the documentation for P.uniformizer().

comment:21 Changed 9 years ago by mstreng

  • Status changed from needs_review to needs_work

Here are some comments have already been mentioned above:

  • why aren't generalized_hilbert_symbol and generalized_even_hilbert_symbol in the same file? (e.g. both in number_field as John suggested)
  • the documentation of generalized_hilbert_symbol says that the prime should be odd, which isn't necessary, in fact, it would be good to have an even example in the doctest so this functionality doesn't get broken
  • the documentation of generalized_even_hilbert_symbol doesn't say that the prime must be even, which it should!

Also, generalized_even_hilbert_symbol is less powerful than the general one:

sage: K.<i> = QuadraticField(-1)                            
sage: O = K.maximal_order()
sage: generalized_hilbert_symbol(K(1/3), K(1), (1+i)*O)     
1
sage: generalized_even_hilbert_symbol(K(1/3), K(1), (1+i)*O)
...
NotImplementedError: inverse_mod is not implemented for non-integral elements

So I guess the documentation of generalized_even_hilbert_symbol should say that the input should consist of integral elements? Possibly the documentation of generalized_even_hilbert_symbol could say that this is simply an auxiliary function and the user should call generalized_hilbert_symbol instead?

needs_work because of the documentation issues for generalized_even_hilbert_symbol

comment:22 Changed 9 years ago by mstreng

  • Reviewers changed from David Loeffler, John Cremona to David Loeffler, John Cremona, Marco Streng

here's a non-documentation reason for needs_work:

sage: K.<i> = QuadraticField(-1)                           
sage: O = K.maximal_order()
sage: generalized_hilbert_symbol(K(-1/3), K(-2/3), (1+i)*O)
...
ValueError: self is not a square root mod P^n

comment:23 Changed 9 years ago by mstreng

And here's another one. In Magma, I get:

> L := QuadraticField(5);
> O := MaximalOrder(L);    
> HilbertSymbol(L!-3, L!-2, 2*O);
1 1/2 + 1/2*i j

In pari, I get:

? k = nfinit(x^2-5)
...
? nfhilbert(k, -3, -2, idealprimedec(k, 2)[1])
%2 = 1

But the patch gives:

sage: L.<a> = QuadraticField(5)
sage: generalized_hilbert_symbol(L(-3), L(-2), 2*O)
-1

Something is wrong...

comment:24 Changed 9 years ago by cremona

In fact, what are the reasons for implementing this independently rather than using pari's function?

Changed 9 years ago by mstreng

interface to pari's nfhilbert, apply only this file

comment:25 Changed 9 years ago by mstreng

  • Authors changed from aly.deines to Aly Deines, Marco Streng
  • Description modified (diff)
  • Work issues changed from ReST formatting issues, and more to bug in Pari 2.4.3

All that needed to be done was wrap pari's nfhilbert function and copy Aly's doctests, but...

One of those doctests revealed yet another bug introduced by the pari upgrade. See http://pari.math.u-bordeaux.fr/cgi-bin/bugreport.cgi?bug=1147

Apply trac_9334_nfhilbert.patch once pari bug 1147 is fixed and that fix reaches Sage.

Possible future improvements:

  • implement also for relative number fields by simply delegating to the absolute case
  • implement also for QQ by wrapping the global function hilbert_symbol as a member of QQ
Note: See TracTickets for help on using tickets.