pierwill / gaussiant

Gaussian integers in Rust.
https://crates.io/crates/gaussiant
1 stars 1 forks source link

Review `is_gaussian_prime` algorithm #10

Open pierwill opened 2 years ago

pierwill commented 2 years ago

I'm not fully confident in the code for this algorithm yet. The integer types don't feel right. And I'm not at all sure the places where it uses absolute values are correct.

JASory commented 2 years ago

For simplicity, rewrite the implementation to GaussianInt<i64> since you are bounded by the largest that primalcheck is able to handle anyway. I don't really understand the choice of checking for zeros since even if (other -3) mod 4 == 0 saves any computation cost (unlikely) it's nowhere near the cost of the primality check you perform beforehand.

Ironically there is a library that you can use that supports all integer types and is the fastest in all intervals but it's not designed to play with the num- libraries since it's forked from a independently unified project.

pierwill commented 2 years ago

Thanks, @JASory! It's always a struggle finding the right level of generality. I agree that limiting this impl GaussianInt<i64> makes sense.

I'm not sure what you mean by "checking for zeros." I pretty much naively implemented the algorithm based on the definition of Gaussian prime on Wikipedia, which is where the (other -3) mod 4 == 0 comes from.

JASory commented 2 years ago

In your test you check conditions for when the norm | components is less than 3 because you subtract 3 from the component and are trying to avoid the condition of checking a negative integer (which you have converted to an unsigned integer). In reality you don't need to check for any of these conditions; simply check the components separately if the other component is zero, and the norm if both components are nonzero.

Also L 184 checks for negative ones, despite the fact that the inputs can never be negative due to the abs function. (And the norm primality check would verify it anyway)