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: sage-8.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:

Status badges

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 Simon Brandhorst

Dependencies: 25023#25023

comment:2 Changed 5 years ago by Simon Brandhorst

I would suggest to make it a method of an element of the number field Suggestions:

  • is_square_in_completion
  • is_local_square

comment:3 Changed 5 years ago by Simon Brandhorst

Branch: u/sbrandhorst/local_square_in_a_number_field

comment:4 Changed 5 years ago by Simon Brandhorst

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:

5c2a982added is_padic_square for numberfields and check keyword

comment:5 Changed 5 years ago by git

Commit: 5c2a982a572477b4cab769717f64d9d43b0ed8536850d7d32ca45abe37323aa9c1bbba12d319a6f8

Branch pushed to git repo; I updated commit sha1. New commits:

6850d7dCleanup and typos

comment:6 Changed 5 years ago by Simon Brandhorst

Authors: Anna HaenschAnna Haensch, Simon Brandhorst
Status: newneeds_review

comment:7 Changed 4 years ago by Dima Pasechnik

Milestone: sage-8.2sage-8.3

I saw the author's cry on twitter, of all things...

comment:8 Changed 4 years ago by Dima Pasechnik

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 4 years ago by Anna Haensch

Replying to dimpase:

I saw the author's cry on twitter, of all things...

Shouting into the void! Sometimes it works!

comment:10 Changed 4 years ago by Dima Pasechnik

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 4 years ago by Anna Haensch

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 Changed 4 years ago by Dima Pasechnik

The "glaring problem" is actually a bug of trac (in the way it interacts with the git server), see https://github.com/sagemath/sagetrac-mirror/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.

Last edited 4 years ago by Dima Pasechnik (previous) (diff)

comment:13 in reply to:  12 ; Changed 4 years ago by Anna Haensch

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/sagetrac-mirror/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 4 years ago by Dima Pasechnik

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/sagetrac-mirror/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 long-standing trac bug.

comment:15 Changed 4 years ago by Dima Pasechnik

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 4 years ago by Anna Haensch

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 Changed 4 years ago by Anna Haensch

@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 a p.
  • I ran the doctests, all tests passed.

comment:18 in reply to:  17 ; Changed 4 years ago by Dima Pasechnik

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 4 years ago by Simon Brandhorst

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 git

Commit: 6850d7d32ca45abe37323aa9c1bbba12d319a6f8ecf748cee4b1dc82bfd2749716f91b1a16ea26d7

Branch pushed to git repo; I updated commit sha1. New commits:

ecf748cP -> p and other small docfixes.

comment:21 Changed 4 years ago by Anna Haensch

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 Changed 4 years ago by Simon Brandhorst

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 4 years ago by Anna Haensch

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 jet-lag talking :/

I think this code looks good and I'm ready to give it a positive review.

comment:24 Changed 4 years ago by Anna Haensch

Status: needs_reviewpositive_review

comment:25 Changed 4 years ago by Volker Braun

Status: positive_reviewneeds_work

Please fill in the reviewer name

comment:26 Changed 4 years ago by Anna Haensch

Reviewers: Anna Haensch, Dima Pasechnik

comment:27 Changed 4 years ago by Anna Haensch

Status: needs_workpositive_review

comment:28 Changed 4 years ago by Volker Braun

Branch: u/sbrandhorst/local_square_in_a_number_fieldecf748cee4b1dc82bfd2749716f91b1a16ea26d7
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.