- changed some asserts to throw - #4566
Conversation
hnil
commented
Mar 29, 2023
hnil
commented
- changing asserts which may happen due to input to throw.
akva2
left a comment
akva2
left a comment
There was a problem hiding this comment.
Looks fine but I would prefer more explicit conditionals.
| resultDelta = gridView.comm().sum(resultDelta); | ||
| resultDenom = gridView.comm().sum(resultDenom); | ||
|
|
||
| if (!(std::isfinite(resultDelta) && std::isfinite(resultDenom))) { |
| ++current_step_; | ||
| current_time_ += dt_; | ||
| assert(dt_ > 0); | ||
| if(!(dt_ > 0)){ |
There was a problem hiding this comment.
<= 0 is more readable.
Arguably yes, but it's not completely equivalent. If, through some unexpected calculation, dt_ happens to be NaN, then <= 0 is false while ! (dt_ > 0) is true. There's also the general advice of avoiding equality comparisons for floating-point numbers if possible.
| // apply max time step if it was set | ||
| dt_ = std::min( dt_estimate, max_time_step_ ); | ||
| assert(dt_ > 0); | ||
| if(!(dt_ > 0)){ |
| dt_ = 0.5 * remaining; | ||
| } | ||
| assert(dt_ > 0); | ||
| if(!(dt_ > 0)){ |
| if( 1.5 * dt_ > remaining ) { | ||
| dt_ = 0.5 * remaining; | ||
| assert(dt_ > 0); | ||
| if(!(dt_ > 0)){ |
| substepTimer.simulationTimeElapsed()); | ||
|
|
||
| assert(dtEstimate > 0); | ||
| if(!(dtEstimate > 0)){ |
| // limit the growth of the timestep size by the growth factor | ||
| dtEstimate = std::min(dtEstimate, double(maxGrowth_ * dt)); | ||
| assert(dtEstimate > 0); | ||
| if(!(dtEstimate > 0)){ |
| } | ||
| assert(dtEstimate > 0); | ||
|
|
||
| if(!(dtEstimate > 0)){ |
| // make sure growth factor is something reasonable | ||
| assert(growthFactor_ >= 1.0); | ||
|
|
||
| if(!(growthFactor_ >= 1.0)){ |
| double SimulatorTimer::stepLengthTaken() const | ||
| { | ||
| assert(current_step_ > 0); | ||
| if(!(current_step_ > 0)){ |
|
Please, pretty please, with lots of sugar on top, use meaningful error messages that help people to see where the problem is. Those used here are 100% correct, but also 100% unable to tell the user in which part of the simulator the problem is. |
|
Closing: the patch targets BlackoilModelEbos.hpp and AdaptiveTimeSteppingEbos.hpp, which no longer exist after the Ebos rename. The underlying issue is still live, so recording it rather than losing it: there are currently 140 assert( calls in opm/simulators/wells/*.cpp. Asserts compile out under NDEBUG, so in a release build those are not checks at all. This is worth revisiting as a fresh change against current master, particularly if opm-common #3427 (WITH_NDEBUG ON by default in release) is ever taken. |
|
Reopening as a draft to keep the issue alive. The patch itself is dead -- BlackoilModelEbos.hpp and AdaptiveTimeSteppingEbos.hpp no longer exist -- but the problem is not: there are currently 140 assert( calls in opm/simulators/wells/*.cpp, and asserts compile out under NDEBUG, so in a release build they are not checks at all. Related to opm-common #3427, which would turn NDEBUG on for release builds; if that lands, converting the load-bearing asserts becomes more urgent. |
|
Superseded by #7355, which reapplies this to today's files - this branch predates the ebos rename and no longer merges. The asserts it targeted are all still on master, so the point stood; it just needed re-siting. |
5 participants
