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.

Sign up for GitHub

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

Jump to bottom

Suggest more-accurate float functions for hand-written formulas #2040

Closed
clarfonthey opened this issue Sep 10, 2017 · 9 comments · Fixed by #5443
Closed

Suggest more-accurate float functions for hand-written formulas #2040

clarfonthey opened this issue Sep 10, 2017 · 9 comments · Fixed by #5443
Labels
A-lint Area: New lints E-medium Call for participation: Medium difficulty level problem and requires some initial experience. good-first-issue These issues are a good way to get started with Clippy L-perf Lint: Belongs in the perf lint group T-middle Type: Probably requires verifiying types

Comments

@clarfonthey
Copy link
Contributor

clarfonthey commented Sep 10, 2017 •
edited
Loading

Last summer I worked with MATLAB code that was copied-down from equations directly, when floating point math allows for a lot of optimisations that aren't always obvious, like exp_m1. Here are a few lints I thought of that specifically apply to floats:

  • a * b + c => a.mul_add(b, c)
  • x.powf(y) => x.powi(y) (when y is an integer)
  • x.powi(2) => x * x
  • 2.powf(x) => x.exp2()
  • x.logN() / y.logN() => x.logbase(y)
  • x.logbase(E) => x.log()
  • x.logbase(10) => x.log10()
  • x.logbase(2) => x.log2().
  • x * PI / 180 => x.to_radians()
  • x * 180 / PI => x.to_degrees()
  • x.powf(1/3) => x.cbrt() (not equivalent, but almost certainly what was meant)
  • x.powf(1/2) => x.sqrt()
  • x.exp() - 1 => x.exp_m1()
  • (x + 1).log() => x.log_1p()
  • sqrt(x * x + y * y) => x.hypot(y) (I see this one used a lot)

Obviously we can't catch every case, and these would be best as hints rather than suggestions, but it'd be nice to at least suggest these options to the user so that they're aware that they exist.

I think that the scope of this should be limited to just suggesting builtin functions for now because the list of mathematical identities is way too large for clippy to incorporate. People who do things like x.log() + y.log() instead of (xy).log() can simplify their equations themselves, but people who don't know that x.hypot(y) exists for example should be made aware.
@oli-obk oli-obk added L-perf Lint: Belongs in the perf lint group good-first-issue These issues are a good way to get started with Clippy E-medium Call for participation: Medium difficulty level problem and requires some initial experience. A-lint Area: New lints T-middle Type: Probably requires verifiying types labels Sep 10, 2017
@Bobo1239
Copy link
Contributor

Just as a sidenote: https://github.com/mcarton/rust-herbie-lint is a project which would also figure out more elaborate transformations but unfortunately hasn't been been update in a year. The actual tool Herbie had a 1.0 release in June so maybe some folks which are familiar with compiler internals could revive rust-herbie-lint. (I failed miserably the last time I tried...)

@clarfonthey
Copy link
Contributor Author

Herbie looks like a fantastic tool; it'd be great if we could leverage that somehow.

@jpetkau
Copy link

jpetkau commented Jun 6, 2018

sqrt(x * x + y * y) => x.hypot(y) (I see this one used a lot)

For good reason: it's almost as accurate as x.hypot(y) in non-overflow cases, and much faster. hypot() jumps through a lot of hoops for the last 0.5 ulp of accuracy, and to give a correct answer when the naive sum would overflow. It's also a non-inlined call to clib, vs. a handful of optimizer-friendly instructions.

(Check out glibc's implementation at https://github.com/lattera/glibc/blob/master/math/w_hypot.c and https://github.com/lattera/glibc/blob/master/sysdeps/ieee754/dbl-64/e_hypot.c to see the difference.)

It would still make sense in a explicit "lint-for-numeric-accuracy" mode (and Herbie would be amazing for that), but questionable for a default lint. The other suggestions all seem like much more likely wins.

@clarfonthey
Copy link
Contributor Author

You do make a good point, @jpetkau, and I think it would make sense to allow configuring the lint to prefer either performance or accuracy.

@krishna-veerareddy krishna-veerareddy mentioned this issue Dec 12, 2019
Merged
bors added a commit that referenced this issue Feb 24, 2020
@bors
Auto merge of #4897 - krishna-veerareddy:issue-2040-accurate-float-fu…
2734e4e 
…nctions, r=flip1995

Add lint to improve floating-point expressions

Looks for floating-point expressions that can be expressed using built-in methods to improve accuracy, performance and/or succinctness.

changelog: Add lint `floating_point_improvements`.

Fixes #4726
Partly addresses [#2040](#2040)

Currently linted expressions:

| Expression | Suggestion |
|---------------------------------|------------|
| x.log(2.0) | x.log2() |
| x.log(10.0) | x.log10() |
| x.log(std::f32::consts::E) | x.ln() |
| (1 + x).ln() | x.ln_1p() |
| (2.0).powf(x) | x.exp2() |
| (std::f32::consts::E).powf(x) | x.exp() |
| x.powf(1/2) | x.sqrt() |
| x.powf(1/3) | x.cbrt() |
| x.powf(y), where y is whole | x.powi(y) |
| x.exp() - 1 | x.exp_m1() |
|x * y + z|x.mul_add(y, z)|
@thiagoarrais
Copy link
Contributor

I'd liike to tackle the remaining suggestions on this issue. As I understand it, they are:

  • x.powi(2) => x * x
  • x.logN() / y.logN() => x.logbase(y)
  • x.logbase(E) => x.log()
  • x.logbase(10) => x.log10()
  • x.logbase(2) => x.log2().
  • x * PI / 180 => x.to_radians()
  • x * 180 / PI => x.to_degrees()
  • (x + 1).log() => x.log_1p()
  • sqrt(x * x + y * y) => x.hypot(y) (I see this one used a lot)

@thiagoarrais thiagoarrais mentioned this issue Apr 9, 2020
Merged
9 tasks
bors added a commit that referenced this issue Jul 13, 2020
@bors
Auto merge of #5443 - thiagoarrais:issue-2040, r=flip1995
3749ea4 
Some accuracy lints for floating point operations

This will add some lints for accuracy on floating point operations suggested by @clarfon in #2040 (fixes #2040).

These are the remaining lints:

- [x] x.powi(2) => x * x
- [x] x.logN() / y.logN() => x.logbase(y)
- [x] x.logbase(E) => x.log()
- [x] x.logbase(10) => x.log10()
- [x] x.logbase(2) => x.log2().
- [x] x * PI / 180 => x.to_radians()
- [x] x * 180 / PI => x.to_degrees()
- [x] (x + 1).log() => x.log_1p()
- [x] sqrt(x * x + y * y) => x.hypot(y)

changelog: Included some accuracy lints for floating point operations
flip1995 added a commit to flip1995/rust-clippy that referenced this issue Jul 13, 2020
@flip1995
Rollup merge of rust-lang#5443 - thiagoarrais:issue-2040, r=flip1995
8db3b92 
Some accuracy lints for floating point operations

This will add some lints for accuracy on floating point operations suggested by @clarfon in rust-lang#2040 (fixes rust-lang#2040).

These are the remaining lints:

- [x] x.powi(2) => x * x
- [x] x.logN() / y.logN() => x.logbase(y)
- [x] x.logbase(E) => x.log()
- [x] x.logbase(10) => x.log10()
- [x] x.logbase(2) => x.log2().
- [x] x * PI / 180 => x.to_radians()
- [x] x * 180 / PI => x.to_degrees()
- [x] (x + 1).log() => x.log_1p()
- [x] sqrt(x * x + y * y) => x.hypot(y)

changelog: Included some accuracy lints for floating point operations
bors added a commit that referenced this issue Jul 13, 2020
@bors
Auto merge of #5443 - thiagoarrais:issue-2040, r=flip1995
52f7940 
Some accuracy lints for floating point operations

This will add some lints for accuracy on floating point operations suggested by @clarfon in #2040 (fixes #2040).

These are the remaining lints:

- [x] x.powi(2) => x * x
- [x] x.logN() / y.logN() => x.logbase(y)
- [x] x.logbase(E) => x.log()
- [x] x.logbase(10) => x.log10()
- [x] x.logbase(2) => x.log2().
- [x] x * PI / 180 => x.to_radians()
- [x] x * 180 / PI => x.to_degrees()
- [x] (x + 1).log() => x.log_1p()
- [x] sqrt(x * x + y * y) => x.hypot(y)

changelog: Included some accuracy lints for floating point operations
@bors bors closed this as completed in 75d43aa Jul 13, 2020
@clarfonthey
Copy link
Contributor Author

Were all of the things listed in the OP implemented in the merged PR? Not sure if this should be kept open or not

@flip1995
Copy link
Member

cc @thiagoarrais do you know if some of the suggestions in the OP are still missing?

@thiagoarrais
Copy link
Contributor

I've tried to tackle all of them, yeah. After #4897 and #5443 (plus some other stuff that made its way into master independently) this should be done.

@clarfonthey
Copy link
Contributor Author

Thanks for checking! Just wanted to make sure nothing got missed. :)

@thiagoarrais thiagoarrais mentioned this issue May 4, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-lint Area: New lints E-medium Call for participation: Medium difficulty level problem and requires some initial experience. good-first-issue These issues are a good way to get started with Clippy L-perf Lint: Belongs in the perf lint group T-middle Type: Probably requires verifiying types
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Some accuracy lints for floating point operations thiagoarrais/rust-clippy
7 participants
@thiagoarrais @oli-obk @jpetkau @Manishearth @Bobo1239 @flip1995 @clarfonthey