Fix metric FD verifier issues #167
Conversation
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
| bool MetricVerifier::IsGreaterThanParameter(long double value) const { | ||
| return value > parameter_ + epsilon; | ||
| } |
There was a problem hiding this comment.
Вот тут сложно. Есть конкретный пример/тест, для которого сравнение нужно делать так? То есть какую конкретно проблему мы решаем
There was a problem hiding this comment.
В cosine метрике расстояния между всеми значениями 0, но с параметром 0 зависимость не выполняется.
Я тест ещё добавил для этого случая.
There was a problem hiding this comment.
но с параметром 0 зависимость не выполняется
Если параметр равен 0, то по определению метрической зависимости она не выполняется?
There was a problem hiding this comment.
Если расстояния между всеми значеиниями 0, то в данном случая при любом параметре должна зависимость выполняться. Но с параметром 0 она не выполнялась. 0 <= 0 давал false
There was a problem hiding this comment.
То есть в сущности проблема в обычном сравнении doubleов. Не уверен, что фиксированный эпсилон это лучший способ тут, хм
There was a problem hiding this comment.
мб посмотреть, как с этим сражается gtest? есть EXPECT_DOUBLE_EQ, например, посмотреть, что там под капотом
https://groups.google.com/g/googletestframework/c/7ZoZWlfxpg4/m/4dHJaUbtWqAJ
|
clang-tidy review says "All clean, LGTM! 👍" |
Fixes floating point comparison errors when comparing values with parameter, adds a test case to cover this issue.
Fixes potential issue with access to highlight_calculator before it is created.