-
Notifications
You must be signed in to change notification settings - Fork 5.3k
refactor: use unitfloat in more places #14396
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
Changes from all commits
b46c1e9
d68720c
d6fccf7
a39e5f9
2388799
5e6758a
d06309e
70295b3
933a157
856a63c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,26 @@ template <typename T, typename Interval> class ClosedIntervalValue { | |
|
|
||
| T value() const { return value_; } | ||
|
|
||
| // Returns a value that is as far from max as the original value is from min. | ||
| // This guarantees that max().invert() == min() and min().invert() == max(). | ||
| ClosedIntervalValue invert() const { | ||
| return ClosedIntervalValue(value_ == Interval::max_value | ||
| ? Interval::min_value | ||
| : value_ == Interval::min_value | ||
| ? Interval::max_value | ||
| : Interval::max_value - (value_ - Interval::min_value)); | ||
| } | ||
|
|
||
| // Comparisons are performed using the same operators on the underlying value | ||
| // type, with the same exactness guarantees. | ||
|
|
||
| bool operator==(ClosedIntervalValue<T, Interval> other) const { return value_ == other.value(); } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any concerns about rounding errors in float comparison?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, the endpoints (0 and 1) are represented exactly by floats. Other code shouldn't use this. I guess that might be an argument for not having == or != ?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that this is just a refactor, I think this isn't any worse than before.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a little paranoid, because later in the PR i see this pattern: Are we then guaranteed that x==z and w==y? In other words, are the exact comparisons guaranteed after there's been arithmetic in the flow? Can you check if you haven't already?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One thing you could consider is offering an isMax() or isMin() method which could abstract a more robust way of doing compares.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, there's no guarantee that 1.0f == (1.0f - 0.0f). I've gone ahead and removed the equality comparison operators in favor of isMin and isMax methods. |
||
| bool operator!=(ClosedIntervalValue<T, Interval> other) const { return value_ != other.value(); } | ||
| bool operator<(ClosedIntervalValue<T, Interval> other) const { return value_ < other.value(); } | ||
| bool operator<=(ClosedIntervalValue<T, Interval> other) const { return value_ <= other.value(); } | ||
| bool operator>=(ClosedIntervalValue<T, Interval> other) const { return value_ >= other.value(); } | ||
| bool operator>(ClosedIntervalValue<T, Interval> other) const { return value_ > other.value(); } | ||
|
|
||
| private: | ||
| T value_; | ||
| }; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any usages of this where the precision downgrade will make a difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this might make a difference of a few milliseconds on a 10 minute timer but that's not worth worrying about.