Add Integer::checked_{log,log2,log10} #70835
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @cramertj (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
In the tweet thread, there's a reference to C++ adding support in C++11, but the C++ function takes an integer parameter and returns a floating-point result. Also, every time I remember actually needing anything like a log of an integer, it was to get the required number of bits to represent a value, so it's not even satisfied directly by this proposal; my use case is easily solved using |
@tspiteri I'm confused by your argument here. This patch includes clear motivation for adding this method. It links to a real-world use case of a database vendor needing this functionality in Rust and expressing getting this right being tedious. The fact that this is a different use case than yours is unfortunate but that doesn't mean this is useless.
That's an interesting statement. I was under the impression that integer operations always floor unless specified otherwise. For example integer division works the same way: // 7/3 = ~2.33 = 2
assert_eq!(7u8 / 3u8, 2u8);The ecosystem contains extensions for e.g. |
I saw the bit about motivation as it could be tricky to get it right if required, but it didn't include a use case. Now going back to the twitter thread I see that there actually is a use case, but I have to scroll up to see it after following the link, so I had missed it the first time round. Looking into it, I see there is
Yeah, I saw that bit of my comment was rather weird so I edited off before I saw your reply, sorry about that. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
I'm glad we've been able to agree on the validity of using
What they're describing here is My understanding is that various encodings such as hex, base32, and base64 may want to calculate similar things as we do in the example for Finally I think it's generally a good idea to maintain API consistency where possible. Floats have access to |
|
Thanks, I find the example of using |
tspiteri
reviewed
Apr 6, 2020
|
An idea about pub fn log10(self) -> Option<Self> {
if val <= 0 {
return None;
}
let size = mem::size_of::<Self>() as u32;
let log2 = size * 8 - 1 - self.leading_zeros();
// Since log2 < 128, multiplying log2 by LOG10_2_TIMES_2_TO_26 never overflows.
// (log10 <= 38, which needs up to 6 bits, leaving 26 bits free in u32.)
const LOG10_2_TIMES_2_TO_26: u32 = 20_201_781;
let lower_bound = (log2 * LOG10_2_TIMES_2_TO_26) >> 26;
let ten_to_lower_bound = Self::pow(10, lower_bound);
if ten_to_lower_bound < Self::MAX / 10 && self >= ten_to_lower_bound * 10 {
Some(lower_bound as Self + 1)
} else {
Some(lower_bound as Self)
}
} |
|
@tspiteri a useful measure would be to run it through compiler explorer and compare the ASM. Here's a link comparing the assembly of both versions: link (your version is shorter and doesn't seem to rely on recursion). However I'm having some trouble making it generic for all integer types; only |
|
Argh, I had previously manually inlined log2 without the conversion to T, and got that working, then I edited the code to use |
tspiteri
reviewed
Apr 6, 2020
8 tasks
|
@tspiteri I'm still having some trouble integrating your code for all number types. Perhaps it'd be an idea if we can land this PR using I've opened a tracking issue in #70887 and marked "optimize log10 implementation" as a todo. |
|
There's some discussion of integer log 10 on Bit Twiddling Hacks: |
|
@cuviper That is quite similar to the function I wrote, with the major difference being that it uses a lookup table instead of There are two other minor differences: I used floor and optionally increased one, the bit hack uses ceil and optionally decreases one; and I used 20201472 >> 26, the bit hack uses 1233 >> 12. |
|
Every single time I've used That said, it's not obvious to me that that |
|
@scottmcm The @yoshuawuyts I think |
|
@yoshuawuyts I've just realized that the log functions are only defined in the That also led me to realize that the tests are not running, otherwise this would have been caught. I think you need to include a line |
|
a perhaps more easy to understand ilog2 implementation would be: fn ilog2(self) -> Option<u32> {
if self <= 0 {
None
} else {
(1 as Self).leading_zeros() - self.leading_zeros()
}
} |
|
While I support adding log functions to integers, I am confused, why the log functions return an integer. assert_eq!(999u32.log(10), Some(2));These functions should return a floating point number like they do in C++ . |
@EdorianDark the C++ docs you linked show floating point log functions. They take a float ( |
|
@yoshuawuyts There are several overloads for clamp including the following: double log ( IntegralType arg ); |
|
This PR is quite different in motivation from the C++ functions mentioned on integral types (even if there is a link in the original PR comment); the C++ functions work by casting the number to double and then finding the logs. For example, with both g++ and clang++, I get the same result for Now since they return double, the error is within the rounding error of floats. But if I understand well, the motivation of this PR is to get the correct result when returning the integer log, so it is quite different. And the equivalent of the C++ functions taking integral arguments would remain for example |
|
I would find it very surprising, if a function called |
|
@EdorianDark I've filed a note on whether the methods should instead be called edit: I think |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
It seems the bors merge failed -- I'm unsure how to identify the problem / how to resolve this? |
|
looks spurious @bors retry |
bors
added
S-waiting-on-bors
|
@Dylan-DPC hmm, failed again |
|
@yoshuawuyts well it is still in the queue :D |
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Jul 9, 2020
Add Integer::checked_{log,log2,log10}
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](https://twitter.com/substack/status/1236445105197727744)_
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](https://www.rapidtables.com/math/algebra/Logarithm.html#log-rules) to arrive at `base10`. However because we're performing intermediate rounding we arrive at the wrong result:
```rust
// 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](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=6bd6c68b3539e400f9ca4fdc6fc2eed0)_
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](https://www.rapidtables.com/math/algebra/Logarithm.html#log-rules)
- [Rounding error playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=6bd6c68b3539e400f9ca4fdc6fc2eed0)
- [substack's tweet asking about integer log2 in the stdlib](https://twitter.com/substack/status/1236445105197727744)
- [Integer Logarithm, A. Jaffer 2008](https://people.csail.mit.edu/jaffer/III/ilog.pdf)
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Jul 9, 2020
Add Integer::checked_{log,log2,log10}
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](https://twitter.com/substack/status/1236445105197727744)_
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](https://www.rapidtables.com/math/algebra/Logarithm.html#log-rules) to arrive at `base10`. However because we're performing intermediate rounding we arrive at the wrong result:
```rust
// 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](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=6bd6c68b3539e400f9ca4fdc6fc2eed0)_
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](https://www.rapidtables.com/math/algebra/Logarithm.html#log-rules)
- [Rounding error playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=6bd6c68b3539e400f9ca4fdc6fc2eed0)
- [substack's tweet asking about integer log2 in the stdlib](https://twitter.com/substack/status/1236445105197727744)
- [Integer Logarithm, A. Jaffer 2008](https://people.csail.mit.edu/jaffer/III/ilog.pdf)
bors
added
S-waiting-on-author
|
It appears to me that the failure is because in the failing android test, Both |
|
Maybe a workaround would be to skip 8192 and 32768 in the |
|
☔ The latest upstream changes (presumably #73265) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I'm going to close this pr due to inactivity. If you wish, you can create a new pull request with these changes and we can work from there. Thanks for contributing |
Dylan-DPC-zz
added
S-inactive
JohnTitor
added a commit
to JohnTitor/rust
that referenced
this pull request
Jul 7, 2021
Add Integer::log variants _This is another attempt at landing rust-lang#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](https://twitter.com/substack/status/1236445105197727744)_ 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](https://www.rapidtables.com/math/algebra/Logarithm.html#log-rules) to arrive at `base10`. However because we're performing intermediate rounding we arrive at the wrong result: ```rust // 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](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=6bd6c68b3539e400f9ca4fdc6fc2eed0)_ 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](https://www.rapidtables.com/math/algebra/Logarithm.html#log-rules) - [Rounding error playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=6bd6c68b3539e400f9ca4fdc6fc2eed0) - [substack's tweet asking about integer log2 in the stdlib](https://twitter.com/substack/status/1236445105197727744) - [Integer Logarithm, A. Jaffer 2008](https://people.csail.mit.edu/jaffer/III/ilog.pdf)
Dylan-DPC
removed
the
S-inactive
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
edit: this PR has moved to #80918
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.
— @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
base10for an integer. We might try and calculate thebase2for the values, and attempt a base swap to arrive atbase10. However because we're performing intermediate rounding we arrive at the wrong result: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 theilogfunction. 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