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

[ER] isqrt() function for integral values #89273

Open
leonardo-m opened this issue Sep 26, 2021 · 13 comments
Open

[ER] isqrt() function for integral values #89273

leonardo-m opened this issue Sep 26, 2021 · 13 comments
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@leonardo-m
Copy link

leonardo-m commented Sep 26, 2021

As in Python 3:
https://docs.python.org/3/library/math.html#math.isqrt

To the Rust stdlib it could be added a isqrt() function for all u*/i*/usize/isize, that returns the floor of the square root of the nonnegative integral value n, that is the greatest integer a such that a*a <= n.

This in Python shows that the usual trick to pass through f64 isn't precise enough for u128:

>>> from math import sqrt, isqrt
>>> N = (2 ** 128) - 1 # u128::MAX
>>> a = int(sqrt(N))
>>> a
18446744073709551616
>>> b = isqrt(N)
>>> b
18446744073709551615
>>> (a * a) <= N
False
@Nicholas-Baron
Copy link
Contributor

@rustbot label: +Libs-small.

@rustbot
Copy link
Collaborator

rustbot commented Oct 11, 2021

Error: Label Libs-small can only be set by Rust team members

Please let @rust-lang/release know if you're having trouble with this bot.

@Nicholas-Baron
Copy link
Contributor

1 question I have is what should be the return value of (-1).isqrt()?
There is no NaN, which is what the floating-point versions return.
Python effectively panics when handed a negative number, but I do not know if that is a good thing for Rust.

I think a working implementation would be a good way to push this further. However, I am just some random contributor and a team member (particularly a library team member) may have a better idea of what to do.

@leonardo-m
Copy link
Author

leonardo-m commented Oct 11, 2021

Python effectively panics when handed a negative number, but I do not know if that is a good thing for Rust.

In Rust we prefer to return Option (or sometimes Result) for partial functions like sqrt (I don't agree with the design of the recently added integral log of the stdlib: "When the number is negative, zero, or if the base is not at least 2; it panics in debug mode and the return value is 0 in release mode.").

A funny API for this function could be:

const fn u8::isqrt() -> u8; // A nibble.
const fn u16::isqrt() -> u8;
const fn u32::isqrt() -> u16;
const fn u64::isqrt() -> u32;
const fn u128::isqrt() -> u64;

const fn i8::isqrt() -> Option<u8>; // A nibble.
const fn i16::isqrt() -> Option<u8>;
const fn i32::isqrt() -> Option<u16>;
const fn i64::isqrt() -> Option<u32>;
const fn i128::isqrt() -> Option<u64>;

(Unfortunately Rust type system doesn't have ranged integral values yet, so the function for u8 needs to return another u8).

But having Option only for the signed ones seems iffy. So an alternative is to use Result<Ty, !> for the unsigned types.

@Alexendoo Alexendoo added C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 12, 2021
@LukasKalbertodt
Copy link
Member

Regarding returning smaller integers than Self: the feature int_log (#70887) has the same open question. This comment suggested to change the return type from T to u32. This was implemented in #88665. This wouldn't work for isqrt as sqrt(2^128 - 1) does not fit into an u32. Maybe we should figure that out for both, log and sqrt, before stabilizing either to increase API consistency?

That said, I'm not sure whether I like returning the smallest possible integer type. For one, as you said, u8 cannot go smaller, making the API somewhat asymmetric. But also, sqrt is an operation that is likely to be chained with other operations in a more complex expression. And returning a smaller integer type could require more casts into those expressions.

But having Option only for the signed ones seems iffy. So an alternative is to use Result<Ty, !> for the unsigned types.

I don't think the signed and unsigned API really needs to be symmetric here. I would much prefer u32::isqrt returning u32 instead of Option<u32> or Result<u32, !>.

@leonardo-m
Copy link
Author

leonardo-m commented Oct 16, 2021

If u32::isqrt returns a u16, and you need a u32 again, you can write u32::from(x.isqrt()) without hard casts and without unwraps. While to go from a u32 result to a u16 you need some kind of hard cast (or try_from). This is one reason why returning types smaller than the input type is nicer.

For usize::isqrt I think it's OK to return another usize.

@wooster0
Copy link
Contributor

I'm not sure if this has been discussed before but I think the name should be sqrt, not isqrt or sqrti because the i doesn't add any value IMO. The same thing has been discussed for {log, log2, log10} in the past and now they are named {log, log2, log10} and not {logi, log2i, log10i} or {ilog, ilog2, ilog10} or {logi, ilogi2, ilogi10}.
And of course {f32, f64}::sqrt isn't called sqrtf or fsqrt either so I don't think it makes sense to call this sqrt variant sqrti or isqrt.

@leonardo-m
Copy link
Author

I named those isqrt because 5.isqrt() doesn't return the square root, it returns the integer truncation of the square root. In Python std lib they have chosen the "isqrt" name. But if for Rust stdlib there's consensus to avoid the leading "i", then it's OK.

@LukasKalbertodt
Copy link
Member

"Integer square root" (abbreviated as isqrt) is an established term in math with a precise definition: https://en.wikipedia.org/wiki/Integer_square_root
So calling the method isqrt is not (just) a Hungarian notation thing.

However, I'm also fine with just sqrt. The only other integer operation (I can think of) that can have a non-integer result is division. And there it's also the truncated/floored/rounded down semantic and not something else like "round to nearest integer". So I'm not sure if calling the integer square root just sqrt would lead to any surprises where people assume other behavior.

@leonardo-m
Copy link
Author

I think it's OK for u8::isqrt to return an u8 plus an assume(result <= 16):

const fn u8::isqrt() -> u8 {
    // ...
    unsafe { assume(result <= 16);
    result
}

Once Rust gains proper well implemented ergonomic ranged integers, that assume() should be removed.

@scottmcm
Copy link
Member

Once we have Integer<0..=N>, isqrt on that should give Integer<0..=⌊√N⌋>.

That said, uN::sqrt should return uN. Analogously to what #88665 (comment) pointed our for log2, that way x.pow(2).isqrt() would return the the same type as x.

@leonardo-m
Copy link
Author

leonardo-m commented Nov 19, 2021

For ranged integers now I've settled to like a syntax like u16[0 ..= N] similar to slicing. It's natural for a Rust programmer and it's compact.

Regarding the type round-trip, I'm content with using a safe looking "from":

let x: u32 = 100;
u32::from(x.pow(2).isqrt())

Rust doesn't have a typeof, otherwise you could write:

type_of(x)::from(x.pow(2).isqrt())

There are situations where you want to use the smaller type returned by the isqrt. Using a smaller type avoids you some casts.

bors added a commit to rust-lang-ci/rust that referenced this issue Sep 28, 2023
Add "integer square root" method to integer primitive types

For every suffix `N` among `8`, `16`, `32`, `64`, `128` and `size`, this PR adds the methods

```rust
const fn uN::isqrt() -> uN;
const fn iN::isqrt() -> iN;
const fn iN::checked_isqrt() -> Option<iN>;
```

to compute the [integer square root](https://en.wikipedia.org/wiki/Integer_square_root), addressing issue rust-lang#89273.

The implementation is based on the [base 2 digit-by-digit algorithm](https://en.wikipedia.org/wiki/Methods_of_computing_square_roots#Binary_numeral_system_(base_2)) on Wikipedia, which after some benchmarking has proved to be faster than both binary search and Heron's/Newton's method. I haven't had the time to understand and port [this code](http://atoms.alife.co.uk/sqrt/SquareRoot.java) based on lookup tables instead, but I'm not sure whether it's worth complicating such a function this much for relatively little benefit.
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 29, 2023
Add "integer square root" method to integer primitive types

For every suffix `N` among `8`, `16`, `32`, `64`, `128` and `size`, this PR adds the methods

```rust
const fn uN::isqrt() -> uN;
const fn iN::isqrt() -> iN;
const fn iN::checked_isqrt() -> Option<iN>;
```

to compute the [integer square root](https://en.wikipedia.org/wiki/Integer_square_root), addressing issue rust-lang#89273.

The implementation is based on the [base 2 digit-by-digit algorithm](https://en.wikipedia.org/wiki/Methods_of_computing_square_roots#Binary_numeral_system_(base_2)) on Wikipedia, which after some benchmarking has proved to be faster than both binary search and Heron's/Newton's method. I haven't had the time to understand and port [this code](http://atoms.alife.co.uk/sqrt/SquareRoot.java) based on lookup tables instead, but I'm not sure whether it's worth complicating such a function this much for relatively little benefit.
@Rudxain
Copy link
Contributor

Rudxain commented Oct 22, 2024

i doesn't add any value IMO.

I think it's more correct, according to the return type. They mean:

  • sqrt: I want the most accurate square-root you can provide, please return a f64 or f128. If you can't return a float, then (at least) please let me know if the result was rounded.
  • isqrt: I just want the floored square-root, I don't care about rounding.

Thus, if we name it sqrt, it should return an Option or Result indicating that the return-value is inexact

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants