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

Linear interpolation #85925

Merged
merged 4 commits into from
Jun 18, 2021
Merged

Linear interpolation #85925

merged 4 commits into from
Jun 18, 2021

Conversation

clarfonthey
Copy link
Contributor

@clarfonthey clarfonthey commented Jun 2, 2021

#71016 is a previous attempt at implementation that was closed by the author. I decided to reuse the feature request issue (#71015) as a tracking issue. A member of the rust-lang org will have to edit the original post to be formatted correctly as I am not the issue's original author.

The common name lerp is used because it is the term used by most code in a wide variety of contexts; it also happens to be the recently chosen name of the function that was added to C++20.

To ensure symmetry as a method, this breaks the usual ordering of the method from lerp(a, b, t) to t.lerp(a, b). This makes the most sense to me personally, and there will definitely be discussion before stabilisation anyway.

Implementing lerp "correctly" is very dififcult even though it's a very common building-block used in all sorts of applications. A good prior reading is this proposal for the C++20 lerp which talks about the various guarantees, which I've simplified down to:

  1. Exactness: (0.0).lerp(start, end) == start and (1.0).lerp(start, end) == end
  2. Consistency: anything.lerp(x, x) == x
  3. Monotonicity: once you go up don't go down

Fun story: the version provided in that proposal, from what I understand, isn't actually monotonic.

I messed around with a lot of different lerp implementations because I kind of got a bit obsessed and I ultimately landed on one that uses the fused mul_add instruction. Floating-point lerp lore is hard to come by, so, just trust me when I say that this ticks all the boxes. I'm only 90% certain that it's monotonic, but I'm sure that people who care deeply about this will be there to discuss before stabilisation.

The main reason for using mul_add is that, in general, it ticks more boxes with fewer branches to be "correct." Although it will be slower on architectures without the fused mul_add, that's becoming more and more rare and I have a feeling that most people who will find themselves needing lerp will also have an efficient mul_add instruction available.

@rust-highfive
Copy link
Collaborator

r? @m-ou-se

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 2, 2021
@the8472
Copy link
Member

the8472 commented Jun 2, 2021

Implementing lerp "correctly" is very dififcult

Then it could benefit from a bunch of test-cases. Are there existing libraries which have collected tricky edge-cases?

@m-ou-se m-ou-se added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 2, 2021
@clarfonthey
Copy link
Contributor Author

You're right about adding tests. I'll try and add a couple over the course of the next week.

@m-ou-se
Copy link
Member

m-ou-se commented Jun 9, 2021

To ensure symmetry as a method, this breaks the usual ordering of the method from lerp(a, b, t) to t.lerp(a, b). This makes the most sense to me personally, and there will definitely be discussion before stabilisation anyway.

I'm not entirely convinced of the t.lerp(a, b) signature. But other signatures (lerp(a, b, t), (a, b).lerp(t), a.lerp(b, t), etc.) also have their problems. So I'd be okay with adding this as unstable, so we can try out how this signature works and get some more feedback.

I decided to reuse the feature request issue (#71015) as a tracking issue. A member of the rust-lang org will have to edit the original post to be formatted correctly as I am not the issue's original author.

Can you open a new tracking issue? Re-using existing issues has some downsides (including the one you mentioned).

library/std/src/f32.rs Outdated Show resolved Hide resolved
library/std/src/f32.rs Outdated Show resolved Hide resolved
/// Values below 0.0 or above 1.0 are allowed, and in general this function closely
/// resembles the value of `start + self * (end - start)`, plus additional guarantees.
///
/// Those guarantees are, assuming that all values are [`finite`]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we give any guarantees about non-finite values? E.g. that any nan wil result in a nan result, and/or that if only one of the values is infinite, the result will be that infinite value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I was considering doing the full list of guarantees that C++ provides, which are:

  • If isfinite(a) && isfinite(b):
    • If t == 0, the result is equal to a.
    • If t == 1, the result is equal to b.
    • If t >= 0 && t <= 1, the result is finite.
  • If isfinite(t) && a == b, the result is equal to a.
  • If isfinite(t) || (!isnan(t) && b-a != 0), the result is not NaN.
  • Let CMP(x,y) be 1 if x > y, -1 if x < y, and 0 otherwise. For any t1 and t2, the product of CMP(lerp(a, b, t2), lerp(a, b, t1)), CMP(t2, t1), and CMP(b, a) is non-negative. (That is, lerp is monotonic.)

But I figured that it might potentially limit what kinds of implementations we allow. I'll try adding tests for these (and comment them out if they don't work) and maybe we can hash it out in the tracking issue.

@clarfonthey clarfonthey mentioned this pull request Jun 13, 2021
5 tasks
@clarfonthey
Copy link
Contributor Author

So, I added another tracking issue, included potential topics of discussion (including calling convention, guarantees), added some extra tests for the C++ guarantees, and modified the docs. Hopefully everything should be good to merge now, but let me know if you think we should have something else!

@m-ou-se
Copy link
Member

m-ou-se commented Jun 17, 2021

@clarfonthey I see you marked the comments as resolved, but the code doesn't seem to be updated. Did your changes not get pushed?

@m-ou-se m-ou-se added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 17, 2021
@clarfonthey
Copy link
Contributor Author

I… did not. Now it is.

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 17, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Jun 17, 2021

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Jun 17, 2021

📌 Commit 525d760 has been approved by m-ou-se

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 17, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 17, 2021
Rollup of 6 pull requests

Successful merges:

 - rust-lang#85925 (Linear interpolation)
 - rust-lang#86202 (Specialize `io::Bytes::size_hint` for more types)
 - rust-lang#86357 (Rely on libc for correct integer types in os/unix/net/ancillary.rs.)
 - rust-lang#86388 (Make `s` pre-interned)
 - rust-lang#86401 (Fix ICE when using `#[doc(keyword = "...")]` on non-items)
 - rust-lang#86405 (Add incr-comp note for 1.53.0 relnotes)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit fcac478 into rust-lang:master Jun 18, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jun 18, 2021
@clarfonthey clarfonthey deleted the lerp branch June 18, 2021 03:50
assert!(below_half <= above_half);

// near 1
let below_one = f32::lerp(1.0 - f32::EPSILON, f32::MIN, f32::MAX);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test fails on targets that use Rust's libm

[dependencies]
libm = "0.2"
mod libc {
    extern "C" {
        pub fn fma(x: f64, y: f64, z: f64) -> f64;
    }
}
fn main() {
    let x = -(1.0 - f64::EPSILON);
    println!("{:?}", libm::fma(x, f64::MIN, f64::MIN));
    println!("{:?}", unsafe { libc::fma(x, f64::MIN, f64::MIN) });
}

Notice you get two different results

Not sure if the test failure is a result of a bug in libm or an implementation error in lerp.

@tmandry
Copy link
Member

tmandry commented Aug 12, 2021

This PR caused crater regressions in the following projects (that were already using lerp via an extension trait):

cc #87749

Not planning to open an issue since we allow this sort of breakage, and it's a small number of crates.

@clarfonthey
Copy link
Contributor Author

cc #86269 since that's the tracking issue for this feature, where such incompatibilities were being discussed.

@mqudsi
Copy link
Contributor

mqudsi commented Nov 16, 2021

For posterity, this was reverted in 6b449b4 (IMHO, the commit should have included a mention of this PR to help those of us chasing down ghosts) as it was causing unstable_name_collisions warnings while there was no consensus on what the final api should look like.

@topolarity
Copy link

A quick note to anyone who might come through here (like me) on a lerp-implementation search.

This implementation is not monotonic after all. A counter example:

f32::lerp(3.6e-07, 0.723387479, 0.929327905) = 0.7233876
f32::lerp(4.8e-07, 0.723387479, 0.929327905) = 0.72338754

@clarfonthey
Copy link
Contributor Author

@topolarity FWIW @CAD97 also was testing this out and had been experimenting in the original tracking issue, if you're curious. It turns out that a proper lerp implementation is quite hard to do; makes you wish that there were just an instruction for it that did the whole thing correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants