Add an error margin when comparing floats. #129721
Conversation
gmarouli
added
auto-backport
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
| } | ||
|
|
||
| for (int i = 0; i < normalizedActual.size(); i++) { | ||
| if (floatsEquals(normalizedActual.get(i), normalizedExpected.get(i))) { |
There was a problem hiding this comment.
So if floatsEquals(...) returns true then there is no match? Maybe rename floatsEquals(...) to floatsNoEquals(...)?
There was a problem hiding this comment.
oh boy, my bad. Actually I prefer to keep it floatsEquals and fix the code because its more intuitive.
...work/src/main/java/org/elasticsearch/datageneration/matchers/source/DynamicFieldMatcher.java
Outdated
Show resolved
Hide resolved
|
Thank you @martijnvg for the review, such an oversight the method name and implementation 🙃 . I run the test locally with What do you think? |
Yes, that should give good confidence. |
| if (normalizedActual.size() != normalizedExpected.size()) { | ||
| return noMatchSupplier.get(); | ||
| } | ||
|
|
||
| for (int i = 0; i < normalizedActual.size(); i++) { | ||
| if (floatEquals(normalizedActual.get(i), normalizedExpected.get(i)) == false) { | ||
| return noMatchSupplier.get(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Both of these checks are on the normalized lists, which have null values removed. This means if there's an extra null somewhere, or if the null is in a different position between the expected and actual, we wouldn't catch it here. Not sure if this is intentional?
There was a problem hiding this comment.
That is correct @jordan-powers , it's not exactly intentional, more preserving existing behaviour.
I have been contemplating about it. The check before was less strict, it only checked for the unique values to be the same, no order, no null, no duplicates. I thought we need to make it stricter at least by checking the order and the values. I wasn't sure about the nulls, mainly, to avoid tripping an NPE, somewhere else.
On the other hand, I guess since we made it stricter, we could take it "all the way".
There was a problem hiding this comment.
One more update, while snooping a little bit more around the code I found this:
It appears that this was intentional. But if I am not mistaken, it doesn't hold anymore, we do preserve order now, right @martijnvg ?
If that is correct, I will open a follow up PR where I address this in all both places and not only when checking doubles. Otherwise, I will revert back to the original so they are in sync.
There was a problem hiding this comment.
We discussed offline, indeed this is not the case anymore. But considering the scope of this PR we do not want to expand to the other normalisers. This can be changed in follow up work.
We chose to keep the non-null values in the given order for the double normalisation because this helps the float comparison with the error margin.
💔 Backport failed
You can use sqren/backport to manually backport by running |
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
We add a margin of error when comparing floats to the DynamicFieldMatcher to account for a small loss of accuracy when loading fields from synthetic source. (cherry picked from commit d859366) # Conflicts: # muted-tests.yml
We add a margin of error when comparing floats to the
DynamicFieldMatcherto account for a small loss of accuracy when loading fields from synthetic source.Furthermore, we switched the set to a list to take the order of array elements also into account since it's supported.
Fixes: #129527, #129528, #129529