Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ZZ[i] and QQ[i] #1168

Merged
merged 13 commits into from
Sep 9, 2021
Merged

Add ZZ[i] and QQ[i] #1168

merged 13 commits into from
Sep 9, 2021

Conversation

tthsqe12
Copy link
Contributor

@tthsqe12 tthsqe12 commented Sep 3, 2021

context: #1110, #1072
I would be nice to have a ZZ[i] and QQ[i] that embed directly (i.e. without user intervention) into the complex numbers.
If you are asking yourself if we really need this, consider the following:

  1. BigInt is to fmpz as Complex{BigInt} is to fmpzi.
  2. Rational{BigInt} is to fmpq as Complex{Rational{BigInt}} is to fmpqi.
  3. fmpzi has a Euclidean structure that we should be able to use
  4. Unlike nf_elem, the fmpzi and fmpqi embed directly into the complex numbers so that conversions and roundings between acb are possible.

If the ZZi and QQi are completely unacceptable, please let me know and I will stop. Otherwise, I will finish the rest of the interface. Also, @thofma, I don't care any more what numerator(::fmpqi) and denominator(::fmpqi) do. They can do whatever you want. And, I don't even care if this is changed:

base_ring(a::FlintQQiField) = ZZi

@codecov
Copy link

codecov bot commented Sep 3, 2021

Codecov Report

Merging #1168 (491054c) into master (4096f8d) will increase coverage by 0.33%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1168      +/-   ##
==========================================
+ Coverage   86.82%   87.16%   +0.33%     
==========================================
  Files          71       74       +3     
  Lines       26681    27332     +651     
==========================================
+ Hits        23166    23824     +658     
+ Misses       3515     3508       -7     
Impacted Files Coverage Δ
src/Nemo.jl 28.40% <ø> (ø)
src/flint/fmpq.jl 93.84% <100.00%> (+0.36%) ⬆️
src/flint/fmpz.jl 89.56% <100.00%> (+1.05%) ⬆️
src/gaussiannumbers/GaussianNumberTypes.jl 100.00% <100.00%> (ø)
src/gaussiannumbers/QQi.jl 100.00% <100.00%> (ø)
src/gaussiannumbers/ZZi.jl 100.00% <100.00%> (ø)
src/calcium/ca.jl 100.00% <0.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4096f8d...491054c. Read the comment docs.

@tthsqe12
Copy link
Contributor Author

tthsqe12 commented Sep 6, 2021

@albinahlback, do you have ideas for the canonicalization of the gcdx coefficients in ZZ[i]?

@albinahlback
Copy link
Contributor

I have a feeling that a generalized Euclidean algorithm should return the canonical coefficients. What I mean is that via these definitions, one should be able to generalize this proof for canonical Bézout coefficients.

@tthsqe12
Copy link
Contributor Author

tthsqe12 commented Sep 6, 2021

I was thinking less of having the canonicity defined by an algorithm and more of having it defined by a set of easy-to-check conditions, like the fmpz_xgcd_canonical_bezout docs. I already have the gcd satsifying -pi/4 < angle(gcd(,)) <= pi/4. so that the gcd is as close the positive real axis as possible.

@albinahlback
Copy link
Contributor

Ah, yeah. Let me think of a definition.

@albinahlback
Copy link
Contributor

albinahlback commented Sep 6, 2021

I think the easiest way to deal with this is to assume that the numbers are coprime (in sense of Gaussian integers).

Let a and b be coprime Gaussian integers such that |a|^2, |b|^2 > 1. Then I am guessing that we should be able to find x and y satisfying a x + b y = 1 such that |x|^2 \le |b|^2 / 2 and |y|^2 \le |a|^2.

And if |a|^2 = 1, then x = 1 / a and y = 0 (and vice versa).

This should be canonical Bézout coefficients. However, if this holds along with that they are unique, I do not know.

@fingolfin
Copy link
Member

Thank you for working on this. I am fine with doing this in principle. As long as there is a good way to convert between these new elements, and existing number field elements?

Also, supporting all the features we have for the isomorphic existing field types. Like, can we also factorize in polynomial rings over QQ[i]?

@tthsqe12
Copy link
Contributor Author

tthsqe12 commented Sep 7, 2021

Yes, I am discussing with thofma how to make it work as a number field/ring.

@tthsqe12 tthsqe12 changed the title [WIP] add ZZ[i] and QQ[i] Add ZZ[i] and QQ[i] Sep 7, 2021
Copy link
Member

@thofma thofma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a bunch of deepcopy, like in conj, which I am not sure about.

According to the documentation here

The results of deepcopy and all arithmetic operations, including powering and
division can be assumed to be new objects without other references being held,
as can objects returned from constructors.

I don't think we should extend this to other random methods. But I am happy to hear your thoughts on this.

src/gaussiannumbers/QQi.jl Outdated Show resolved Hide resolved
src/gaussiannumbers/ZZi.jl Outdated Show resolved Hide resolved
src/gaussiannumbers/ZZi.jl Outdated Show resolved Hide resolved
src/gaussiannumbers/ZZi.jl Outdated Show resolved Hide resolved
src/gaussiannumbers/ZZi.jl Outdated Show resolved Hide resolved
@tthsqe12
Copy link
Contributor Author

tthsqe12 commented Sep 8, 2021

I don't know. I was thinking of real, imag and conj as arithmetic operations. But, if they are not "on the list" then I will not copy here. Also, what counts as a constructor? It is well-known that all over the place the parent(obj) does not necessarily create a new object.

@tthsqe12
Copy link
Contributor Author

tthsqe12 commented Sep 9, 2021

I think is a good start. Pros:

  1. diff coverage is 100%
  2. there is no documentation added whatsoever

Cons:

  1. It is currently slow. I will turn on the jets later.
  2. Maybe it is worth documenting that the rings ZZi and QQi exist?

@thofma
Copy link
Member

thofma commented Sep 9, 2021

Yay! Let's get this in.

  1. Maybe it is worth documenting that the rings ZZi and QQi exist?

Yes, please do this (in a separate PR).

@thofma thofma merged commit 1bfe11e into Nemocas:master Sep 9, 2021
@tthsqe12 tthsqe12 deleted the add_ZZiQQi branch December 9, 2021 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants