Add Integer::log variants

[yoshuawuyts]

This is another attempt at landing #70835, which was approved by the libs team but failed on Android tests through Bors. The text copied here is from the original issue. The only change made so far is the addition of non-checked_ variants of the log methods.

Tracking issue: #70887

---

This implements {log,log2,log10} methods for all integer types. The implementation was provided by @substack for use in the stdlib.

Note: I'm not big on math, so this PR is a best effort written with limited knowledge. It's likely I'll be getting things wrong, but happy to learn and correct. Please bare with me.

Motivation

Calculating the logarithm of a number is a generally useful operation. Currently the stdlib only provides implementations for floats, which means that if we want to calculate the logarithm for an integer we have to cast it to a float and then back to an int.

would be nice if there was an integer log2 instead of having to either use the f32 version or leading_zeros() which i have to verify the results of every time to be sure

— @substack, 2020-03-08

At higher numbers converting from an integer to a float we also risk overflows. This means that Rust currently only provides log operations for a limited set of integers.

The process of doing log operations by converting between floats and integers is also prone to rounding errors. In the following example we're trying to calculate base10 for an integer. We might try and calculate the base2 for the values, and attempt a base swap to arrive at base10. However because we're performing intermediate rounding we arrive at the wrong result:

// log10(900) = ~2.95 = 2
dbg!(900f32.log10() as u64);
    
// log base change rule: logb(x) = logc(x) / logc(b)
// log2(900) / log2(10) = 9/3 = 3
dbg!((900f32.log2() as u64) / (10f32.log2() as u64));

playground

This is somewhat nuanced as a lot of the time it'll work well, but in real world code this could lead to some hard to track bugs. By providing correct log implementations directly on integers we can help prevent errors around this.

Implementation notes

I checked whether LLVM intrinsics existed before implementing this, and none exist yet. Also I couldn't really find a better way to write the ilog function. One option would be to make it a private method on the number, but I didn't see any precedent for that. I also didn't know where to best place the tests, so I added them to the bottom of the file. Even though they might seem like quite a lot they take no time to execute.

References

• Log rules
• Rounding error playground
• substack's tweet asking about integer log2 in the stdlib
• Integer Logarithm, A. Jaffer 2008

[rust-highfive]

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

[yoshuawuyts]

This patch still needs to implement the workaround mentioned by @tspiteri in #70835 (comment):

Maybe a workaround would be to skip 8192 and 32768 in the 1..=u16::MAX loop and 8192 in the 1..=i16::MAX loop, and add explicit checks for those three cases outside the loops without using f32::log2 (together with a comment that f32::log2 is not used in those cases because of android).

The tests are expected to pass CI, but fail once merged and Bors runs.

[yoshuawuyts]

Okay, applied the fix from mentioned above. Fingers crossed this passes now.

[yoshuawuyts]

Addressed the feedback from @nagisa and @the8472

[CDirkx]

All of these methods could also be made const; that would only depend on const_option for Option::unwrap, which I think is okay?

[bors]

☔ The latest upstream changes (presumably #78259) made this pull request unmergeable. Please resolve the merge conflicts.

[yoshuawuyts]

Fixed the merge conflict and applied @CDirkx's feedback: all methods are now const.

[EdorianDark]

I am against the names, since they are very confusing.

Lets look againg at what other languages do for log on an integer:

C++: integer is cast to double and then log is called
C: integer is cast to double and then log is called
Python: uses the C functions
Javascript: had only floats for a long time. The recently introduced BigInt does not have log.
Java: int is implicitly converted to double and then log called
C#: int is implicitly converted to double and then log called

Is there any example to call the integer logarithm log?
I think the naming issue should be resolved before merging this.

[the8472]

I think the naming issue should be resolved before merging this.

The methods are unstable, so this could also be discussed before stabilization.

[yoshuawuyts]

@EdorianDark you've expressed that position repeatedly in earlier threads; namely: #70887 (comment) and #70835 (comment). We concluded in #70835 (comment) that naming should not be a blocker for landing this PR (I commented, you +1'd it).

I don't believe much has changed in the interim, and still hold the position that we don't need to finalize the names in order to move forward with adding this functionality to nightly.

[EdorianDark]

@yoshuawuyts i changed my mind.
The only thing what will change after this is merged, is that code will be written using these functions.
This would it make renaming it even harder.

I feel that in the tracking issue wasn't enough discussion about the names.

Of cause my opinion isn't that important since I am not in any Rust team.

[m-ou-se]

This would produce a very generic panic message in the non-checked_ versions. None of the other methods on integers produce a panic message like that. All of them originate in a panic from one of the primitive operations (/, +, .. etc.).

Except for division by zero (which would be undefined behaviour), all other panics on the integer methods and operators only happen in debug mode. In release mode, they wrap around or return zero. For example, next_power_of_two's documentation states:

        /// When return value overflows (i.e., `self > (1 << (N-1))` for type
        /// `uN`), it panics in debug mode and return value is wrapped to 0 in
        /// release mode (the only situation in which method can return 0).

Should we keep that consistency?

[bors]

☔ The latest upstream changes (presumably #81578) made this pull request unmergeable. Please resolve the merge conflicts.

[yoshuawuyts]

Should we keep that consistency?

I hadn't considered this, and I think we absolutely should.

---

For now I've updated the PR with the other feedback and fixed the merge conflict. But I believe what we should then do is:

• In debug mode .expect with an error which explains what's going on.
• In non-debug .or(0) the numbers so in case of None we round to zero.

Is that accurate?

[yoshuawuyts]

cc/ @m-ou-se this should be ready for another review when you have time

[yoshuawuyts]

Was halfway through making dinner when I realized that we could just turn off the overflow errors here, evading the use of Add::add and re-enabling these functions as const fn. In fact it seems something similar has been done before in f238148. Glad I managed to tick that extra box! :D

[m-ou-se]

Looks good to me! (After #80918 (comment) is fixed.)

Can you update the tracking issue to use our new template? That includes a short overview of the public API.

[m-ou-se]

@bors r+

Thanks for making this happen, @yoshuawuyts!

[m-ou-se]

@bors ping?

[m-ou-se]

@bors r+

[bors]

📌 Commit 9f57996 has been approved by m-ou-se

[bors]

⌛ Testing commit 9f57996 with merge c5e344f...

[mstankus]

Question: I am new to rust and am a mathematician.
log base 2 of 0 is either not defined or NaN.
log base 2 of 1 through 255 is between 0 and 7.
Why are the log2, log10 and log functions return u32?

Thanks.

[EdorianDark]

Question: I am new to rust and am a mathematician. log base 2 of 0 is either not defined or NaN. log base 2 of 1 through 255 is between 0 and 7. Why are the log2, log10 and log functions return u32?

Thanks.

This merge has been closed for a while. A better place to discuss things would be at the tracking issue.