Opened 5 years ago
Closed 4 years ago
#25029 closed enhancement (fixed)
local_square in a number field
Reported by:  Anna Haensch  Owned by:  

Priority:  major  Milestone:  sage8.3 
Component:  number fields  Keywords:  
Cc:  Merged in:  
Authors:  Anna Haensch, Simon Brandhorst  Reviewers:  Anna Haensch, Dima Pasechnik 
Report Upstream:  N/A  Work issues:  
Branch:  ecf748c (Commits, GitHub, GitLab)  Commit:  ecf748cee4b1dc82bfd2749716f91b1a16ea26d7 
Dependencies:  #25023  Stopgaps: 
Description
function returns true
if a is a local square in the completion of a number field at a prime. This function uses quadratic_defect and therefore this ticket is dependent on ticket #25023.
Change History (28)
comment:1 Changed 5 years ago by
Dependencies:  25023 → #25023 

comment:2 Changed 5 years ago by
comment:3 Changed 5 years ago by
Branch:  → u/sbrandhorst/local_square_in_a_number_field 

comment:4 Changed 5 years ago by
Commit:  → 5c2a982a572477b4cab769717f64d9d43b0ed853 

I settled for is_padic_square
as that exists already for QQ.
one might think about giving it the alias is_square_in_completion
to make it more visible?
New commits:
5c2a982  added is_padic_square for numberfields and check keyword

comment:5 Changed 5 years ago by
Commit:  5c2a982a572477b4cab769717f64d9d43b0ed853 → 6850d7d32ca45abe37323aa9c1bbba12d319a6f8 

Branch pushed to git repo; I updated commit sha1. New commits:
6850d7d  Cleanup and typos

comment:6 Changed 5 years ago by
Authors:  Anna Haensch → Anna Haensch, Simon Brandhorst 

Status:  new → needs_review 
comment:7 followup: 9 Changed 4 years ago by
Milestone:  sage8.2 → sage8.3 

I saw the author's cry on twitter, of all things...
comment:8 Changed 4 years ago by
What's going on in the branch, I wonder. It does remove sage/rings/rational_field.py, doesn't it?
comment:9 Changed 4 years ago by
Replying to dimpase:
I saw the author's cry on twitter, of all things...
Shouting into the void! Sometimes it works!
comment:10 followup: 11 Changed 4 years ago by
Is there any reason you cannot do the review between the two authors, it's completely kosher in most cases (unless it's something really major etc), you know?
I'd gladly review the technical part, but the maths behind it is far from my immediate areas.
comment:11 Changed 4 years ago by
Replying to dimpase:
Is there any reason you cannot do the review between the two authors, it's completely kosher in most cases (unless it's something really major etc), you know?
I'd gladly review the technical part, but the maths behind it is far from my immediate areas.
Is that so? If that's the case, I can easily do the review myself. I'm in the process of fixing that glaring problem you point out above. But if you think it's ok to review between the two of us, then you can step down.
comment:12 followup: 13 Changed 4 years ago by
The "glaring problem" is actually a bug of trac (in the way it interacts with the git server), see https://github.com/sagemath/sagetracmirror/blob/u/sbrandhorst/local_square_in_a_number_field/src/sage/rings/rational_field.py
the file is not deleted, it's alive and well.
comment:13 followup: 14 Changed 4 years ago by
Replying to dimpase:
The "glaring problem" is actually a bug of trac (in the way it interacts with the git server), see https://github.com/sagemath/sagetracmirror/blob/u/sbrandhorst/local_square_in_a_number_field/src/sage/rings/rational_field.py
the file is not deleted, it's alive and well.
I had wondered how that slipped through. Strange.
comment:14 Changed 4 years ago by
Replying to annahaensch:
Replying to dimpase:
The "glaring problem" is actually a bug of trac (in the way it interacts with the git server), see https://github.com/sagemath/sagetracmirror/blob/u/sbrandhorst/local_square_in_a_number_field/src/sage/rings/rational_field.py
the file is not deleted, it's alive and well.
I had wondered how that slipped through. Strange.
it's not the first time I see such things. Probably a longstanding trac bug.
comment:15 followup: 16 Changed 4 years ago by
anyhow, it's not something to worry about here. It only affects the branch display on this ticket, nothing else.
comment:16 Changed 4 years ago by
Replying to dimpase:
anyhow, it's not something to worry about here. It only affects the branch display on this ticket, nothing else.
Thanks for the heads up on that.
comment:17 followup: 18 Changed 4 years ago by
@sbrandhorst at the suggestion of dimpase I'm going to review this ticket. Some comments:
 Sorry, but I'm not sure I understand the utility of the check=true variable in the function, what is that doing?
 In the modifications you added to quadratic defect there is a
P
that should be ap
.  I ran the doctests, all tests passed.
comment:18 followup: 19 Changed 4 years ago by
Replying to annahaensch:
@sbrandhorst at the suggestion of dimpase I'm going to review this ticket. Some comments:
 Sorry, but I'm not sure I understand the utility of the check=true variable in the function, what is that doing?
Assuming you talk about
if check and not p.is_prime():
this is to check that the input makes sense. Potentially, primality check is expensive, and so for performance reasons one might want to avoid it.
comment:19 Changed 4 years ago by
Replying to dimpase:
Replying to annahaensch:
@sbrandhorst at the suggestion of dimpase I'm going to review this ticket. Some comments:
 Sorry, but I'm not sure I understand the utility of the check=true variable in the function, what is that doing?
Assuming you talk about
if check and not p.is_prime():this is to check that the input makes sense. Potentially, primality check is expensive, and so for performance reasons one might want to avoid it.
Yep. I am mostly working with small primes. But who know's what other people might do with this?
comment:20 Changed 4 years ago by
Commit:  6850d7d32ca45abe37323aa9c1bbba12d319a6f8 → ecf748cee4b1dc82bfd2749716f91b1a16ea26d7 

Branch pushed to git repo; I updated commit sha1. New commits:
ecf748c  P > p and other small docfixes.

comment:21 Changed 4 years ago by
Great, that makes sense.
Another small thing, depending on the types of fields I construct I'm getting errors of the type
AttributeError: 'sage.rings.number_field.number_field_element_quadratic.NumberFieldElement_quadratic' object has no attribute 'is_padic_square'
or
AttributeError: 'sage.rings.number_field.number_field_element.NumberFieldElement_absolute' object has no attribute 'is_padic_square'
does this mean the function just needs to be added onto some call list?
comment:22 followup: 23 Changed 4 years ago by
I cannot reproduce this
sage: k.<a> = NumberField(x^3+2) sage: a.is_padic_square(k.primes_above(2)[0]) False sage: type(a) <type 'sage.rings.number_field.number_field_element.NumberFieldElement_absolute'> sage: k.<a> = NumberField(x^2+2) sage: a.is_padic_square(k.primes_above(2)[0]) False sage: type(a) <type 'sage.rings.number_field.number_field_element_quadratic.NumberFieldElement_quadratic'>
Are you on the right branch? :)
comment:23 Changed 4 years ago by
Replying to sbrandhorst:
I cannot reproduce this
sage: k.<a> = NumberField(x^3+2) sage: a.is_padic_square(k.primes_above(2)[0]) False sage: type(a) <type 'sage.rings.number_field.number_field_element.NumberFieldElement_absolute'> sage: k.<a> = NumberField(x^2+2) sage: a.is_padic_square(k.primes_above(2)[0]) False sage: type(a) <type 'sage.rings.number_field.number_field_element_quadratic.NumberFieldElement_quadratic'>Are you on the right branch? :)
Oh geez, I am, but I forgot to build Sage. It's the jetlag talking :/
I think this code looks good and I'm ready to give it a positive review.
comment:24 Changed 4 years ago by
Status:  needs_review → positive_review 

comment:25 Changed 4 years ago by
Status:  positive_review → needs_work 

Please fill in the reviewer name
comment:26 Changed 4 years ago by
Reviewers:  → Anna Haensch, Dima Pasechnik 

comment:27 Changed 4 years ago by
Status:  needs_work → positive_review 

comment:28 Changed 4 years ago by
Branch:  u/sbrandhorst/local_square_in_a_number_field → ecf748cee4b1dc82bfd2749716f91b1a16ea26d7 

Resolution:  → fixed 
Status:  positive_review → closed 
I would suggest to make it a method of an element of the number field Suggestions:
is_square_in_completion
is_local_square