Better NaN handling for fastMin and fastMax #559
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `fastMin` and `fastMax` functions previously didn't taken NaN into account, so returned inaccurate results when NaNs were passed in, biasing NaN to be returned only if it were the first argument. This change makes the behaviour of the functions symmetrical, returning NaN if _either_ argument (or both) are NaN. Benchmarks have been added to show that indeed the fast variants are significantly faster than their standard variants. ``` $ go test -run=XXX -bench='Fast|Math' ./geom goos: linux goarch: arm64 pkg: github.com/peterstace/simplefeatures/geom BenchmarkFastMin-2 712878825 1.574 ns/op BenchmarkFastMax-2 1000000000 1.101 ns/op BenchmarkMathMin-2 149962322 7.861 ns/op BenchmarkMathMax-2 152543700 7.866 ns/op PASS ``` The motivation behind this change is to remove the error from Envelope constructors, and perform validation separately as its own method (similar to the recent changes from geometries). Since the envelope only stores the min and max points, I'd like any NaNs to "infect" the envelope so that they're caught by validation rather than silently being dropped.
albertteoh
approved these changes
Oct 9, 2023
| @@ -69,15 +70,15 @@ func sequenceToXYs(seq Sequence) []XY { | |||
|
|
|||
| // fastMin is a faster but not functionally identical version of math.Min. | |||
| func fastMin(a, b float64) float64 { | |||
| if a < b { | |||
| if math.IsNaN(a) || a < b { | |||
There was a problem hiding this comment.
Nice, looks to also be consistent behaviour with the built-in min (and max) funcs offered in Go 1.21.
Do we need to handle +-Inf as well?
There was a problem hiding this comment.
Yes, once I upgrade to go 1.21, we might not need these anymore. Probably not going to upgrade though until Go 1.23 comes out, in order to not force users to upgrade early.
It doesn't need to handle infinity explicitly, since it's handled implicitly by the a < b check. All of the cases involving infinity "just work" 😁.
Base automatically changed from
use_sequence_envelope_instead_of_extend
to
master
October 15, 2023 18:26
|
Thanks for reviewing @albertteoh 🙇 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The
fastMinandfastMaxfunctions previously didn't taken NaN into account, so returned inaccurate results when NaNs were passed in, biasing NaN to be returned only if it were the first argument.This change makes the behaviour of the functions symmetrical, returning NaN if either argument (or both) are NaN.
Benchmarks have been added to show that indeed the fast variants are significantly faster than their standard variants.
The motivation behind this change is to remove the error from Envelope constructors, and perform validation separately as its own method (similar to the recent changes from geometries). Since the envelope only stores the min and max points, I'd like any NaNs to "infect" the envelope so that they're caught by validation rather than silently being dropped.
Check List
Have you:
Added unit tests? Yes.
Add cmprefimpl tests? (if appropriate?) N/A
Updated release notes? (if appropriate?) N/A
Related Issue