Conversation
ohhmm
force-pushed
the
less
branch
2 times, most recently
from
5a3b59a to
8e527d7
Compare
ohhmm
force-pushed
the
less
branch
2 times, most recently
from
0b79006 to
50e4730
Compare
ohhmm
force-pushed
the
less
branch
5 times, most recently
from
86f672b to
223abba
Compare
ohhmm
force-pushed
the
less
branch
2 times, most recently
from
b5824db to
97ad974
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| BOOST_AUTO_TEST_CASE(Less_operator_expression_test) { | ||
| auto LessOperatorExpression = X.Less(Y); | ||
| std::cout << "X<Y : " << LessOperatorExpression << std::endl; | ||
| for (auto x = -1.; x<1.; x+=.1) { | ||
| for (auto y = -1.; y<1.; y+=.1) { | ||
| auto isLess = x < y; | ||
| auto lessOperatorInstantiation = LessOperatorExpression; | ||
| Valuable::vars_cont_t evalMap = {{X, x}, {Y, y}}; | ||
| std::cout << '\n' << x << "<" << y << " = "; | ||
| lessOperatorInstantiation.eval(evalMap); | ||
| std::cout << " expression that must be equal to zero when true: " << lessOperatorInstantiation | ||
| << std::endl; | ||
|
|
||
| lessOperatorInstantiation.optimize(); | ||
| std::cout << std::endl << "Is " << x << "<" << y << " : " << lessOperatorInstantiation << std::endl; | ||
| bool b = {}; | ||
| auto boolLessOp = lessOperatorInstantiation.ToBool(); | ||
| BOOST_TEST(boolLessOp == isLess); | ||
| boolLessOp.eval(evalMap); | ||
| BOOST_TEST(boolLessOp == isLess); | ||
| if (boolLessOp == true) { | ||
| BOOST_TEST(boolLessOp.IsInt()); | ||
| BOOST_TEST(lessOperatorInstantiation.IsInt()); | ||
| b = lessOperatorInstantiation.IsInt() && lessOperatorInstantiation.ca() == 0; | ||
| std::cout << std::endl << x << "<" << y << " : " << b << std::endl; | ||
| } else if (boolLessOp == false) { | ||
| BOOST_TEST(boolLessOp.IsInt()); | ||
| b = lessOperatorInstantiation == 0; | ||
| std::cout << std::endl | ||
| << x << "<" << y << " : " << (b ? "true; " : "false; ") | ||
| << lessOperatorInstantiation << " != 0" << std::endl; | ||
| } else { | ||
| std::cout << std::endl << x << "<" << y << " : " << boolLessOp << std::endl; | ||
| BOOST_TEST(!"boolLessOp must have boolean value"); | ||
| } | ||
| BOOST_TEST(boolLessOp == b); | ||
|
|
||
| auto ok = b == isLess; | ||
| if (!ok) { | ||
| std::cout << "X=" << x << " Y=" << y << ' ' << ok << " bool: " << b << std::endl; | ||
| BOOST_TEST(ok); | ||
| } | ||
| } |
There was a problem hiding this comment.
I've reviewed the PR and found several issues that should be addressed:
-
Code Duplication: The new
Less_operator_expression_test(lines 406-448) is very similar to the existingLess_operator_test(lines 462-504). Consider refactoring to avoid duplication, perhaps by creating a helper function that both tests can use. -
Floating-Point Precision Issues: In
Less_operator_expression_test, the loop uses increments of 0.1 (line 408-409):
for (auto x = -1.; x<1.; x+=.1) {
for (auto y = -1.; y<1.; y+=.1) {This can lead to floating-point precision issues. Consider using a more robust approach like defining a specific set of test values or using a different increment value.
- Inconsistent Comparison Methods: In the new test, you're using direct comparison with 0 (line 430):
b = lessOperatorInstantiation == 0;But in the modified Less_operator_test, you've changed to use IsZero() (line 534):
b = lessOperatorInstantiation.IsZero();For consistency, the new test should also use IsZero().
ohhmm
force-pushed
the
less
branch
3 times, most recently
from
59bfa9c to
849ca15
Compare
ohhmm
force-pushed
the
less
branch
2 times, most recently
from
60673d4 to
b93f3cc
Compare
| BOOST_AUTO_TEST_CASE(Less_operator_expression_test) { | ||
| auto LessOperatorExpression = X.Less(Y); | ||
| std::cout << "X<Y : " << LessOperatorExpression << std::endl; | ||
| for (auto x = -1.; x<1.; x+=.1) { | ||
| for (auto y = -1.; y<1.; y+=.1) { | ||
| auto isLess = x < y; | ||
| auto lessOperatorInstantiation = LessOperatorExpression; | ||
| Valuable::vars_cont_t evalMap = {{X, x}, {Y, y}}; | ||
| std::cout << '\n' << x << "<" << y << " = "; | ||
| lessOperatorInstantiation.eval(evalMap); | ||
| std::cout << " expression that must be equal to zero when true: " << lessOperatorInstantiation | ||
| << std::endl; | ||
|
|
||
| lessOperatorInstantiation.optimize(); | ||
| std::cout << std::endl << "Is " << x << "<" << y << " : " << lessOperatorInstantiation << std::endl; | ||
| bool b = {}; | ||
| auto boolLessOp = lessOperatorInstantiation.ToBool(); | ||
| BOOST_TEST(boolLessOp == isLess); | ||
| boolLessOp.eval(evalMap); | ||
| BOOST_TEST(boolLessOp == isLess); | ||
| if (boolLessOp == true) { | ||
| BOOST_TEST(boolLessOp.IsInt()); | ||
| BOOST_TEST(lessOperatorInstantiation.IsInt()); | ||
| b = lessOperatorInstantiation.IsInt() && lessOperatorInstantiation.ca() == 0; | ||
| std::cout << std::endl << x << "<" << y << " : " << b << std::endl; | ||
| } else if (boolLessOp == false) { | ||
| BOOST_TEST(boolLessOp.IsInt()); | ||
| b = lessOperatorInstantiation == 0; | ||
| std::cout << std::endl | ||
| << x << "<" << y << " : " << (b ? "true; " : "false; ") | ||
| << lessOperatorInstantiation << " != 0" << std::endl; | ||
| } else { | ||
| std::cout << std::endl << x << "<" << y << " : " << boolLessOp << std::endl; | ||
| BOOST_TEST(!"boolLessOp must have boolean value"); | ||
| } | ||
| BOOST_TEST(boolLessOp == b); | ||
|
|
||
| auto ok = b == isLess; | ||
| if (!ok) { | ||
| std::cout << "X=" << x << " Y=" << y << ' ' << ok << " bool: " << b << std::endl; | ||
| BOOST_TEST(ok); | ||
| } | ||
| } |
There was a problem hiding this comment.
I've identified several issues in the PR:
-
Code Duplication: The new
Less_operator_expression_test(lines 406-448) duplicates much of the existingLess_operator_test(lines 462-504). Consider refactoring to avoid duplication by creating a helper function that both tests can use. -
Floating-Point Precision Issues: In
Less_operator_expression_test, the loop uses increments of 0.1:
for (auto x = -1.; x<1.; x+=.1) {
for (auto y = -1.; y<1.; y+=.1) {This can lead to floating-point precision issues. Consider using a more robust approach like defining a specific set of test values or using a different increment value.
- Inconsistent Comparison Methods: In the new test, you're using direct comparison with 0 (line 430):
b = lessOperatorInstantiation == 0;But in the modified Less_operator_test, you've changed to use IsZero() (line 487):
b = lessOperatorInstantiation.IsZero();For consistency, the new test should also use IsZero().
ohhmm
force-pushed
the
less
branch
3 times, most recently
from
a253bb9 to
c52e002
Compare
1 participant
No description provided.