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

Add Float.Extra.[equalWithin,interpolateFrom] #54

Conversation

ianmackenzie
Copy link
Contributor

One thing to consider: should Float.Extra.equalWithin 1 (1 / 0) (1 / 0) return True, i.e. should infinite be considered (exactly) equal to itself? The current code will always return false when comparing two infinities since the difference between two infinities is not well defined...but this is arguably inconsistent with aboutEqual which does consider infinities of the same sign to be "about equal". (You could maybe argue that's because aboutEqual uses relative instead of absolute tolerance, but I think it would also make sense to change aboutEqual to always return false for infinities.)

@miniBill
Copy link
Collaborator

My gut feeling is that they should be equal (if of the same sign ofc)

@gampleman
Copy link
Collaborator

I'd also be for having these functions treat infinities as equal, especially since (1/0) == (1/0).

Comment on lines 458 to 462
if parameter <= 0.5 then
start + parameter * (end - start)

else
end + (1 - parameter) * (start - end)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the reason for the if statement? I'd have thought that just start + parameter * (end - start) would be pretty standard lerp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My rationale was that start + parameter * (end - start) could (due to roundoff) lead to situations where interpolateFrom a b 1.0 would not give you exactly b, which seemed like it would be confusing/surprising. The version with the if will recover b exactly in that case, since 1 - parameter factor will be exactly zero. (It should also have better numerical accuracy for parameter values close to 1.)

That said, I can't actually imagine a concrete use case where this would cause a problem.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Of course that has to be weighed against the tiny performance cost of the extra comparison. I'd probably opt for removing it.

--> True

-}
equalWithin : Float -> Float -> Float -> Bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

I kind of wonder if a elm-test style type Tolerance = Relative Float | Absolute Float | RelativeOrAbsolute Float Float wouldn't be nicer here as the first argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps...I've always been suspicious of relative tolerances myself, but that may be because I personally often work with values that are positions/coordinates instead of fundamentally being magnitudes. When working with positions it really doesn't make sense to use a relative tolerance, since a "small" value is just one that happens to be close to your chosen origin point, which is arbitrary. (Said another way, if I'm comparing the positions of two points in space to see if they're approximately equal, that comparison should not depend on my choice of origin point - but if I use a relative tolerance to compare coordinate values, then the comparison will depend on the choice of origin point.)

My hunch is that in most cases it's better to choose a tolerance value based on some higher-level context than individual points/values (e.g. the size of a geometric bounding box, or more generally some nominal value representing the general scale of things that you're working with), and then use that fixed tolerance as an 'absolute' tolerance for all comparisons. (If relative tolerances are so great, then why do you usually have to combine them with a 'fudge factor' for comparisons near zero?)

Of course, all that said, with an API like you propose people like me can just choose to use Absolute and others can decide for themselves if Relative or RelativeOrAbsolute is appropriate for their use case 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On that note...if we do use a relative tolerance, should it be based on abs firstValue or max (abs firstValue) (abs secondValue)?

  • The first case (as used currently by aboutEqual) has the advantage that all comparisons against the same reference value can use the same tolerance
  • The second case has the advantage that the comparison is symmetric, e.g. aboutEqual a b will always return the same result as aboutEqual b a, which is not true of the current implementation

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a great comment and an excellent explanation! I think the best action really would be to adapt it into extra documentation for aboutEqual as guidance on the correctness advantages of switching to equalWithin and how to choose a correct tolerance parameter.

Consistent with aboutEqual, although I still think it feels weird
@ianmackenzie
Copy link
Contributor Author

OK I've updated the code to return equal for (same-sign) infinities - I don't love it, but if you're trying to compare infinities for approximate equality something's probably already gone wrong 😅

Better performance at the cost of a small amount of numerical stability
@gampleman
Copy link
Collaborator

I've merged this manually. If there are any further changes then let's make them as separate PRs.

@gampleman gampleman closed this Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants