Opened 19 months ago
Last modified 3 days ago
#24098 positive_review enhancement
S_hilbert_symbol() helper function to find integer with negative Hilbert symbol for primes in a set S
Reported by:  annahaensch  Owned by:  

Priority:  minor  Milestone:  sage8.8 
Component:  basic arithmetic  Keywords:  sd90 
Cc:  mroy, sbrudzin, jduque  Merged in:  
Authors:  Simon Brandhorst, Anna Haensch, Manami Roy, Juanita Duque, Sandi Rudzinski  Reviewers:  Simon Brandhorst, Anna Haensch 
Report Upstream:  N/A  Work issues:  
Branch:  u/annahaensch/_s_hilbert_symbol___helper_function_to_find_integer_with_negative_hilbert_symbol_for_primes_in_at_set_s (Commits)  Commit:  bca5e62b90d0e84b086b412cd6cabe4a244038ca 
Dependencies:  #25023,#25029  Stopgaps: 
Description (last modified by )
This is an implementation of algorithm 3.4.1 from Markus Kirschmer's "Definite quadratic and hermitian forms with small class number." Given a nonsquare b and a set of primes S, the function returns a number a which has negative Hilbert symbol with respect to b at precisely the primes in S.
This is a function in service to algorithm 3.4.3, quadratic_form_form_local_invariants()
, ticket #24108.
Change History (36)
comment:1 Changed 19 months ago by
 Description modified (diff)
 Summary changed from _S_hilbert_symbol() helper function to find integer with negative Hilbert symbol for primes in at set S to S_hilbert_symbol() helper function to find integer with negative Hilbert symbol for primes in at set S
comment:2 Changed 19 months ago by
 Branch set to u/annahaensch/_s_hilbert_symbol___helper_function_to_find_integer_with_negative_hilbert_symbol_for_primes_in_at_set_s
comment:3 Changed 19 months ago by
 Commit set to ec55c0d42702b1bf86f12e9e0cdb13f3c76949d2
 Status changed from new to needs_review
comment:4 Changed 19 months ago by
 Cc manami.roy.90@… sbrudzin@… jduque@… added
comment:5 Changed 19 months ago by
 Cc mroy sbrudzin jduque added; manami.roy.90@… sbrudzin@… jduque@… removed
comment:6 Changed 19 months ago by
 Component changed from quadratic forms to basic arithmetic
comment:7 Changed 19 months ago by
 Keywords sd90 added; #sd90 removed
 Summary changed from S_hilbert_symbol() helper function to find integer with negative Hilbert symbol for primes in at set S to S_hilbert_symbol() helper function to find integer with negative Hilbert symbol for primes in a set S
comment:8 Changed 18 months ago by
 Description modified (diff)
 Reviewers set to Simon Brandhorst
comment:9 Changed 18 months ago by
So here are some thoughts:
This is just a helper function so it should not pollute the global namespace.
Is there a reason that you have implemented Markus algorithm just for QQ
?
The beauty of Markus' work is that he did it directly for number fields.
It should not be too hard to do it for number fields.
All the necessary functions like class groups, ideals, hilbert symbols, places, are there:
sage: K.<a>=CyclotomicField(13) sage: P=K.primes_above(3)[0] sage: P Fractional ideal (3, a^3  a  1) sage: K.hilbert_symbol(3,1+a,P) 1
We can define the function as a method of the number field. Just like the hilbert_symbol. Maybe the name S_hilbert_symbol is misleading? I would expect it to be the global hilbert_symbol of the localization at S or something like that. This function seems more like an inverse image? maybe hilbert_symbol_inv_S?
Some remarks for formatting: Comments should be of the form (note the space)
# foo does bla foo = 3
See https://www.python.org/dev/peps/pep0008/#comments.
You are redefining V,W,vv in the while loop. But really they stay constant. So they should not sit in the loop.
Use the names of Markus if possible or more descriptive names. HH,PP etc. is not very descriptive. Same for i,j. I do not see the need to define H. Why hide everything in lists?
Either use the verbose things or remove them.
comment:10 Changed 18 months ago by
It is good to test the error messages. But they belong to the TESTS:: block not the EXAMPLES:: block. > they do not need to show up in the documentation.
comment:11 Changed 18 months ago by
 Status changed from needs_review to needs_work
comment:12 Changed 13 months ago by
 Dependencies set to #25023
comment:13 Changed 13 months ago by
 Branch changed from u/annahaensch/_s_hilbert_symbol___helper_function_to_find_integer_with_negative_hilbert_symbol_for_primes_in_at_set_s to u/sbrandhorst/_s_hilbert_symbol___helper_function_to_find_integer_with_negative_hilbert_symbol_for_primes_in_at_set_s
comment:14 followup: ↓ 18 Changed 13 months ago by
 Branch changed from u/sbrandhorst/_s_hilbert_symbol___helper_function_to_find_integer_with_negative_hilbert_symbol_for_primes_in_at_set_s to u/annahaensch/_s_hilbert_symbol___helper_function_to_find_integer_with_negative_hilbert_symbol_for_primes_in_at_set_s
 Status changed from needs_work to needs_review
How about hilbert_conductor_inverse
as a name?
Problem is this exists and is (allmost) the same function.
Ideas? I would like to move it as a method of QQ
.
comment:15 Changed 13 months ago by
 Branch changed from u/annahaensch/_s_hilbert_symbol___helper_function_to_find_integer_with_negative_hilbert_symbol_for_primes_in_at_set_s to u/sbrandhorst/_s_hilbert_symbol___helper_function_to_find_integer_with_negative_hilbert_symbol_for_primes_in_at_set_s
comment:16 Changed 13 months ago by
 Commit changed from ec55c0d42702b1bf86f12e9e0cdb13f3c76949d2 to 61faa15afef6589f0a8131cbf593c0078f89ce9f
Branch pushed to git repo; I updated commit sha1. New commits:
61faa15  Clean up

comment:17 Changed 13 months ago by
comment:18 in reply to: ↑ 14 Changed 13 months ago by
Replying to sbrandhorst:
How about
hilbert_conductor_inverse
as a name?
I'm not sure about that name, it already exists and means something else. I am happy with S_hilbert_symbol since it's restricting the Hilbert symbol at a finite set of primes, it's pretty consistent with your initial interpretation in comment 9; a hilbert symbol taken at a finite number of localizations.
Problem is this exists and is (allmost) the same function. Ideas? I would like to move it as a method of
Eventually we can split this into two methods, one for QQ an one for number fields. I've got the general number fields method queued up and ready to go as soon as tickets #25023 and #25029 are positively reviewed.
comment:19 Changed 13 months ago by
Mhh I do not like the prefix S_. In methods like S_unit_group and S_class_group, the primes in S are inverted. But we are not inverting anything  we are working in fields anyways. So there is no relation to this kind of objects? Finally in these Srings one can define a hilbert symbol and we are not computing it.
For b
and S
given our function computes a
such that hilbert_conductor(a,b) = prod(S)
plus a condition at infinity. So I think that it is pretty close. And I like the idea that if you type
hilbert
+ autocompletion you get this method suggested.
So maybe
hilbert_something
hilbert_conductor_something
hilbert_conductor_inverse
with optional argumentb
?hilbert_conductor_inverse_S
?
ooops I have the numberfield version ready as well
comment:20 Changed 13 months ago by
How about hilbert_symbol_positive_at_S
?
comment:21 followup: ↓ 22 Changed 13 months ago by
i could settle for hilbert_symbol_negative_at_S
:)
comment:22 in reply to: ↑ 21 Changed 13 months ago by
Replying to sbrandhorst:
i could settle for hilbert_symbol_negative_at_S
:)
Ah, yes, even better. Let's go with that :)
comment:23 Changed 13 months ago by
 Dependencies changed from #25023 to #25023,#25029
comment:24 Changed 13 months ago by
 Commit changed from 61faa15afef6589f0a8131cbf593c0078f89ce9f to 707703320e734ba38a61a28cbc179b07f5165aa8
Branch pushed to git repo; I updated commit sha1. New commits:
7077033  changed name to hilbert_symbol_negative_at_S

comment:25 Changed 10 months ago by
Would you review the number field version? Else we can split it off to another ticket.
comment:26 Changed 12 days ago by
This ticket has been asleep for awhile, but let's try and finish it. I just tried to run doctests on and got an error message
AttributeError: Values instance has no attribute 'gc'
Any ideas?
comment:27 Changed 10 days ago by
 Commit changed from 707703320e734ba38a61a28cbc179b07f5165aa8 to 1fbeefce060bfcffd765b4b0827a7e6a85339eec
Branch pushed to git repo; I updated commit sha1. New commits:
9e5a98f  Merge branch 'develop' into t/24098/_s_hilbert_symbol___helper_function_to_find_integer_with_negative_hilbert_symbol_for_primes_in_at_set_s

b7ca8fe  fixed doctests

1fbeefc  Merge branch 'develop' into t/24098/_s_hilbert_symbol___helper_function_to_find_integer_with_negative_hilbert_symbol_for_primes_in_at_set_s

comment:28 Changed 10 days ago by
should be okay now.
comment:29 Changed 10 days ago by
 Commit changed from 1fbeefce060bfcffd765b4b0827a7e6a85339eec to 1a6e6da35a2950911847943dbb1f6e9e513c2574
Branch pushed to git repo; I updated commit sha1. New commits:
1a6e6da  removed unnecessary import

comment:30 followup: ↓ 31 Changed 7 days ago by
We have a green bot. Tests pass.
comment:31 in reply to: ↑ 30 Changed 6 days ago by
Replying to sbrandhorst:
We have a green bot. Tests pass.
All tests pass! I had just run them independently anyhow, but good to know :)
comment:32 Changed 4 days ago by
 Branch changed from u/sbrandhorst/_s_hilbert_symbol___helper_function_to_find_integer_with_negative_hilbert_symbol_for_primes_in_at_set_s to u/annahaensch/_s_hilbert_symbol___helper_function_to_find_integer_with_negative_hilbert_symbol_for_primes_in_at_set_s
comment:33 Changed 4 days ago by
 Commit changed from 1a6e6da35a2950911847943dbb1f6e9e513c2574 to bca5e62b90d0e84b086b412cd6cabe4a244038ca
comment:34 Changed 4 days ago by
Add your name as a reviewer, then we can set positive review.
comment:35 Changed 4 days ago by
 Reviewers changed from Simon Brandhorst to Simon Brandhorst, Anna Haensch
 Status changed from needs_review to positive_review
comment:36 Changed 3 days ago by
 Milestone changed from sage8.1 to sage8.8
New commits:
Adds function S_hilbert_symbol