-
Notifications
You must be signed in to change notification settings - Fork 85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: two hard crash fixes #3382
base: develop
Are you sure you want to change the base?
Conversation
@@ -783,12 +783,14 @@ MultiFluidBase::KernelWrapper:: | |||
real64 totalMolality = 0.0; | |||
for( integer ic = 0; ic < numComps; ++ic ) | |||
{ | |||
GEOS_ERROR_IF( m_componentMolarWeight[ic] < LvArray::NumericLimits< real64 >::epsilon, "Zero component molecular weight" ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m_componentMolarWeight
is user input so we should validate it in postInputInitialization
MultiFluidBase.cpp:184 instead of in this kernel function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved the check there, thanks
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3382 +/- ##
========================================
Coverage 56.79% 56.79%
========================================
Files 1076 1076
Lines 95739 95742 +3
========================================
+ Hits 54375 54378 +3
Misses 41364 41364 ☔ View full report in Codecov by Sentry. |
@corbett5, @cssherman, and/or @rrsettgast could you please review? |
@@ -54,7 +54,7 @@ EventManager::EventManager( string const & name, | |||
setDescription( "Start simulation time for the global event loop." ); | |||
|
|||
registerWrapper( viewKeyStruct::maxTimeString(), &m_maxTime ). | |||
setApplyDefaultValue( std::numeric_limits< real64 >::max() ). | |||
setApplyDefaultValue( std::numeric_limits< integer >::max() * units::YearSeconds ). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could even use a smaller number tbh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we shouldn't use a number based on std::numeric_limits< T >::max()
. While the actual value of std::numeric_limits< real64 >::max()
in seconds is around 5.7e300 years
, this seems like a lot given the age of the universe is 13e9 years
, and the age of the earth is 4e9
years. Perhaps a nice number would be 1e6 years = 3.1536e+13 seconds
?
src/coreComponents/constitutive/fluid/multifluid/MultiFluidBase.hpp
Outdated
Show resolved
Hide resolved
…e.hpp Co-authored-by: Matteo Cusini <[email protected]>
@corbett5, @cssherman, and/or @rrsettgast please review, the change is minor and the issue appeared after FTE were enabled |
real64 const mwInv = 1.0 / m_componentMolarWeight[ic]; | ||
compMoleFrac[ic] = composition[ic] * mwInv; // this is molality (units of mole/mass) | ||
dCompMoleFrac_dCompMassFrac[ic][ic] = mwInv; | ||
totalMolality += compMoleFrac[ic]; | ||
} | ||
|
||
GEOS_ERROR_IF( totalMolality < LvArray::NumericLimits< real64 >::epsilon, "Zero total molality, all component concentrations are equal to zero." ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may go bad prior to totalMolality < LvArray::NumericLimits< real64 >::epsilon
?? If so perhaps a higher number here? Also can we have a graceful recovery somehow? For example if totalMolality ~= 0
then that means that each compMoleFrac[ic] ~= 0
, which means that composition[ic] ~=0
? Should we put a limit on the minimum value of the primary variables? We are talking about void at this point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all elements composition
can not be zero, the issue originates somewhere in the initialization (from user input) where the check is missing
@@ -54,7 +54,7 @@ EventManager::EventManager( string const & name, | |||
setDescription( "Start simulation time for the global event loop." ); | |||
|
|||
registerWrapper( viewKeyStruct::maxTimeString(), &m_maxTime ). | |||
setApplyDefaultValue( std::numeric_limits< real64 >::max() ). | |||
setApplyDefaultValue( std::numeric_limits< integer >::max() * units::YearSeconds ). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we shouldn't use a number based on std::numeric_limits< T >::max()
. While the actual value of std::numeric_limits< real64 >::max()
in seconds is around 5.7e300 years
, this seems like a lot given the age of the universe is 13e9 years
, and the age of the earth is 4e9
years. Perhaps a nice number would be 1e6 years = 3.1536e+13 seconds
?
@rrsettgast the issue was that that maxTime get's converted into number of years as integer number and then overflows, that's why i put that fix |
one is when all component concentrations are zeros - division by zero
and another is when maxTime is not specified for events - there is overflow when huge double is caster into int