Resolve unreachable code in cut kernel - #2121
Conversation
7305ef9 to
d512912
Compare
maxfirmbach
left a comment
maxfirmbach
left a comment
There was a problem hiding this comment.
@georghammerl I think the changes make a lot of sense. I would propose to go forward and fix the failing tests.
| } | ||
|
|
||
| if (det < 1.0e-16 && det > 1.0e-16) | ||
| if (det < -1.0e-16) |
There was a problem hiding this comment.
Should we use a numerical limit here?
| msg << "Determinant in compute position is negative: " << det; | ||
| FOUR_C_THROW("{}", msg.str()); | ||
| } | ||
|
|
| { | ||
| std::stringstream msg; | ||
| msg << "Determinant in compute position is negative: " << det; | ||
| FOUR_C_THROW("{}", msg.str()); |
There was a problem hiding this comment.
FOUR_C_THROW can format strings directly
| FOUR_C_THROW("{}", msg.str()); | |
| FOUR_C_THROW("Determinant in compute position is negative: {}", det); |
There was a problem hiding this comment.
Yes and no.
A std::formatter<T> specialization is neccessary for every {} argument type. Since FloatType is a template parameter that can be instantiated with things like double, Sacado::Fad::DFad<double>, or Core::CLN::ClnWrapper, there is no generic formatter for it. To circumvent adding those at the moment, the workaround with the operator<< which is available for all template types.
|
@georghammerl should we merge this soon? because I have the same problem in #1795 |
|
I would like to merge, however the tests behave different and I have no clue on those details that matter in the cut code :-/ |
|
Hi @georghammerl, I made the fix in #2161 and seems this check does not affect the test. I would suggest to merge #2161 and rebase this PR based on this for further adjustment. The only fix in #2161 is needed to continue with #1795. |
4 participants
Description and Context
The Intel compiler pointed me to this line in the cut kernel code
4C/src/cut/4C_cut_kernel.hpp
Line 2203 in 55490c3
which evaluates always to false.
I am no expert of this piece of code. Please review my proposed solution.
The failing tests reveal that the proposed changes lead to different behavior. Tests with negative determinant seem to have passed in the past?!
Alternative: Delete the lines of code as they have never been reached in the past.