Add lint to improve floating-point expressions

[krishna-veerareddy]

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

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)

[krishna-veerareddy]

@clarfon Hey I wanted to get the ball rolling on this issue but I have a few questions regarding how we would separate the lints. For example, should I create separate lints for log(2|10|e) and log_1p functions or should I group all the log functions under a single lint or should I group all the suggestions under a single lint?

The direction I was going in was creating separate lints for each of your suggestions but I am not sure if that's the right approach.

[clarfonthey]

Wouldn't this be the same as one of the known constant lints? Don't see why this would be lumped under a different lint.

We could maybe even lint common trig function calls which return values of already provided constants, e.g. sin(PI) == 0. But maybe calling it a "computed known constant" rather than a literal without a name is better, and splitting that from the approx literal lint.

Ignore me. I can't read.

[krishna-veerareddy]

No, this would lint calls such as (1f32).log(2.0) and suggest usage of (1f32).log2() instead. It does the same for bases 10 and e. known_log_bases was probably a poor choice for the lint name. Specifically this PR fixes these three items in your issue under a single lint:

x.logbase(E) => x.log()
x.logbase(10) => x.log10()
x.logbase(2) => x.log2()

[clarfonthey]

Oh! I did that thing I absolutely shouldn't do which is to assume something without reading the whole thing. -___-

Honestly, I feel like one lint should cover all cases for things like this. Things that have explicit specialised versions should go in one lint. (With the exception of things like powf(1/3) -> cbrt, which aren't the same but the latter is probably intended.)

Things which don't have explicit functions but are more accurate should go in a separate lint, e.g. Herbie's home page example of sqrt(x+1) - sqrt(x) being better off as 1/(sqrt(x+1) + sqrt(x)).

[clarfonthey]

Another potential thing to consider is whether you're optimising for speed or accuracy. I'd maybe separate lints based upon that, e.g. hypot is always more accurate but may be slower.

[krishna-veerareddy]

@clarfon Hey I created two lints: one for better accuracy and another for performance. The expression currently linted are:

x.log(2) => x.log2() // for better accuracy
x.log(10) => x.log10() // for better accuracy
x.log(E) => x.ln() // for better accuracy
(1 + x).ln() => x.ln_1p() // for better accuracy
(2.0).powf(x) => x.exp2() // for better performance
E.powf(x) => x.exp() // for better performance
x.powf(1 / 2) => x.sqrt() // for better performance
x.powf(1 / 3) => x.cbrt() // for better performance
x.exp() - 1 => x.exp_m1() // for better accuracy

I will add lints for some of the other suggestions in your issue in a separate PR. Would love to know your thoughts/suggestions.

[clarfonthey]

Honestly, it may be worth a "always better" lint for some. For example, the log lints are pretty much the same or better for performance and better for accuracy. Ditto for the log plus one and exp minus one lints. However, changing to hypot or fused multiply-add might reduce performance, and thus is better for accuracy only.

[krishna-veerareddy]

Yeah makes sense. Any suggestion for the lint name?

[llogiq]

How about floating_point_perf and floating_point_accuracy?

Thinking about it more, we likely want the best perf and accuracy, but some people will accept perf loss for more accuracy while others will accept suboptimal accuracy for better perf.

So we can either make both lints and for suggestions that don't exacerbate perf or accuracy for the respective other we lint the more likely lint unless it's disabled. Or we make a floating_point_improvements lint that will improve both perf without accuracy loss and accuracy without perf loss.

[krishna-veerareddy]

@llogiq Any suggestion for lints that are better for both performance and accuracy? Should I use floating_point_perf_and_accuracy or is that too verbose.

[krishna-veerareddy]

Having a floating_point_improvements lint for better accuracy and performance sounds good to me. Should this lint also cover cases where there is no clear accuracy/performance improvement but makes the code more idiomatic or should that be a separate lint? Here are a couple examples:

x * PI / 180 => x.to_radians()
x * 180 / PI => x.to_degrees()
x.logN() / y.logN() => x.logbase(y)

[llogiq]

I'd put everything that sacrifices accuracy under floating_point_performance, everything that sacrifices performance under floating_point_accuracy and everything else under floating_point_improvements.

[krishna-veerareddy]

I didn't have to add the floating_point_accuracy and floating_point_performance lints yet but please let me know if any of the methods in this PR would sacrifice either performance or accuracy and Ill add that method under the relevant lint.

[bors]

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

[flip1995]

I agree with the grouping of the lints, but I don't like the names. E.g., #[allow(floating_point_improvements)] is kind of misleading and against our lint naming convention (cc #2845).

My suggestions:

• accuracy: inaccurate_floating_points
• perf: bad_performing_floating_points
• general: bad_floating_points

I'm not happy with the word bad though. But writing allow(lint_name) should describe (in an English "sentence") what is allowed there:

#[allow(clippy::inaccurate_floating_point)]
let y = x.log(2);

So it is obvious, that x.log(2) is inaccurate, but acceptable there, whereas allow(floating_point_accuracy) kind of suggests the opposite.

[krishna-veerareddy]

Makes sense. How about:
general: {inferior|substandard|ill_advised|misguided}_floating_points
accuracy: inaccurate_floating_points
performance: {slow|inefficient}_floating_points

Also having a floating_point_{operation|computation|arithmetic|expression} suffix makes more sense to me than floating_points although it does make the lint name long.

[flip1995]

I like inefficient_floating_points as a lint name for the perf group. For the general case I guess inferior is the best word here.

As for the suffix, I personally like operations best.

[clarfonthey]

Could always just do flops for floating point operations

[krishna-veerareddy]

That is more concise but isn't flops supposed to be floating point operations per second? So maybe just flop? I am happy with flops too but don't want to cause any confusion.

The similar term FLOP is often used for floating-point operation, for example as a unit of counting floating-point operations carried out by an algorithm or computer hardware. - Wikipedia

[llogiq]

Yes, {inefficient|inaccurate|substandard}_flop looks acceptable to me.

[krishna-veerareddy]

@flip1995 Hey I changed the name of the lint to suboptimal_flops as we discussed above. The reason you don't see the other lints yet(inefficient_flops and imprecise_flops) were that all the checks above improve either accuracy or performance or both without loss of either.

But, I looked at the implementation of these library functions and some of these are internally calling the cmath library which could go under the imprecise_flops category as the cmath implementation sacrifices performance for accuracy. Here are the methods that use cmath internally: cbrt, ln_1p, exp_m1 and hypot(this one isn't linted yet). I can start making this change but would love to know your thoughts.

Also, I temporarily removed the lints to improve style as there were some bugs in implementation. Ill create a separate PR for those.

[flip1995]

The reason you don't see the other lints yet [...] were that all the checks above improve either accuracy or performance or both without loss of either.

Ah ok, that's fair. As long as it is in nursery, it doesn't matter too much anyway.

I can start making this change but would love to know your thoughts.

I think this is part of the split up, we should do, when we "stabilize" the lint(s)

Also, I temporarily removed the lints to improve style as there were some bugs in implementation.

👍

---

I'd say with the NITs fixed, I noted above, we can merge this, and everything else can be done in a later PR(s) 👍

[krishna-veerareddy]

@flip1995 Hey I created the imprecise_flops lint and moved some suggestions under that lint since I already had the changes. Also, fixed all the above comments. Please review when you have some time. Thanks!

[flip1995]

@bors r+

Thanks!

[bors]

📌 Commit ff0d44e has been approved by flip1995

[bors]

⌛ Testing commit ff0d44e with merge 2734e4e...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 2734e4e to master...