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: 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 4 years ago by sbrandhorst

  • Dependencies changed from 25023 to #25023

comment:2 Changed 4 years ago by sbrandhorst

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

  • Branch set to u/sbrandhorst/local_square_in_a_number_field

comment:4 Changed 4 years ago by sbrandhorst

  • 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:

5c2a982added is_padic_square for numberfields and check keyword

comment:5 Changed 4 years ago by git

  • Commit changed from 5c2a982a572477b4cab769717f64d9d43b0ed853 to 6850d7d32ca45abe37323aa9c1bbba12d319a6f8

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

6850d7dCleanup and typos

comment:6 Changed 4 years ago by sbrandhorst

  • Authors changed from Anna Haensch to Anna Haensch, Simon Brandhorst
  • Status changed from new to needs_review

comment:7 follow-up: Changed 3 years ago by dimpase

  • Milestone changed from sage-8.2 to sage-8.3

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

comment:8 Changed 3 years ago by dimpase

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 annahaensch

Replying to dimpase:

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

Shouting into the void! Sometimes it works!

comment:10 follow-up: Changed 3 years ago by 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.

comment:11 in reply to: ↑ 10 Changed 3 years ago by annahaensch

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 follow-up: Changed 3 years ago by dimpase

The "glaring problem" is actually be 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.

Version 1, edited 3 years ago by dimpase (previous) (next) (diff)

comment:13 in reply to: ↑ 12 ; follow-up: Changed 3 years ago by 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.

comment:14 in reply to: ↑ 13 Changed 3 years ago by dimpase

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 follow-up: Changed 3 years ago by dimpase

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 annahaensch

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 follow-up: Changed 3 years ago by 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?
  • 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 ; follow-up: Changed 3 years ago by 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.

comment:19 in reply to: ↑ 18 Changed 3 years ago by sbrandhorst

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 git

  • Commit changed from 6850d7d32ca45abe37323aa9c1bbba12d319a6f8 to ecf748cee4b1dc82bfd2749716f91b1a16ea26d7

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

ecf748cP -> p and other small docfixes.

comment:21 Changed 3 years ago by annahaensch

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 follow-up: Changed 3 years ago by 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? :-)

comment:23 in reply to: ↑ 22 Changed 3 years ago by annahaensch

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 3 years ago by annahaensch

  • Status changed from needs_review to positive_review

comment:25 Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work

Please fill in the reviewer name

comment:26 Changed 3 years ago by annahaensch

  • Reviewers set to Anna Haensch, Dima Pasechnik

comment:27 Changed 3 years ago by annahaensch

  • Status changed from needs_work to positive_review

comment:28 Changed 3 years ago by vbraun

  • Branch changed from u/sbrandhorst/local_square_in_a_number_field to ecf748cee4b1dc82bfd2749716f91b1a16ea26d7
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.