Opened 4 years ago
Closed 3 years ago
#25029 closed enhancement (fixed)
local_square in a number field
Reported by:  annahaensch  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 4 years ago by
 Dependencies changed from 25023 to #25023
comment:2 Changed 4 years ago by
comment:3 Changed 4 years ago by
 Branch set to u/sbrandhorst/local_square_in_a_number_field
comment:4 Changed 4 years ago by
 Commit set to 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 4 years ago by
 Commit changed from 5c2a982a572477b4cab769717f64d9d43b0ed853 to 6850d7d32ca45abe37323aa9c1bbba12d319a6f8
Branch pushed to git repo; I updated commit sha1. New commits:
6850d7d  Cleanup and typos

comment:6 Changed 4 years ago by
 Status changed from new to needs_review
comment:7 followup: ↓ 9 Changed 3 years ago by
 Milestone changed from sage8.2 to sage8.3
I saw the author's cry on twitter, of all things...
comment:8 Changed 3 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 in reply to: ↑ 7 Changed 3 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 3 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 in reply to: ↑ 10 Changed 3 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 3 years ago by
The "glaring problem" is actually be 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 in reply to: ↑ 12 ; followup: ↓ 14 Changed 3 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 in reply to: ↑ 13 Changed 3 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 3 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 in reply to: ↑ 15 Changed 3 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 3 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 in reply to: ↑ 17 ; followup: ↓ 19 Changed 3 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 in reply to: ↑ 18 Changed 3 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 3 years ago by
 Commit changed from 6850d7d32ca45abe37323aa9c1bbba12d319a6f8 to ecf748cee4b1dc82bfd2749716f91b1a16ea26d7
Branch pushed to git repo; I updated commit sha1. New commits:
ecf748c  P > p and other small docfixes.

comment:21 Changed 3 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 3 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 in reply to: ↑ 22 Changed 3 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 3 years ago by
 Status changed from needs_review to positive_review
comment:25 Changed 3 years ago by
 Status changed from positive_review to needs_work
Please fill in the reviewer name
comment:26 Changed 3 years ago by
 Reviewers set to Anna Haensch, Dima Pasechnik
comment:27 Changed 3 years ago by
 Status changed from needs_work to positive_review
comment:28 Changed 3 years ago by
 Branch changed from u/sbrandhorst/local_square_in_a_number_field to ecf748cee4b1dc82bfd2749716f91b1a16ea26d7
 Resolution set to fixed
 Status changed from positive_review to closed
I would suggest to make it a method of an element of the number field Suggestions:
is_square_in_completion
is_local_square