Fix source statistics#4056
Conversation
dkachuma
added
ci: run CUDA builds
| void SourceFluxStatsAggregator::WrappedStats::combine( WrappedStats const & other ) | ||
| { | ||
| stats().combine( other.stats() ); | ||
| m_statsPeriodStart = LvArray::math::max( m_statsPeriodStart, other.m_statsPeriodStart ); |
There was a problem hiding this comment.
Shouldn't it be LvArray::math::min?
There was a problem hiding this comment.
Some of the processes lag behind because the time doesn't get advanced. So we need to pick the most advanced time.
There was a problem hiding this comment.
OK, I just would add a short comment to explain this choice.
| // produce the period stats of this rank | ||
| m_stats.m_elementCount = m_periodStats.m_elementCount; | ||
| m_statsPeriodStart = m_periodStats.m_periodStart; | ||
| m_statsPeriodStart = MpiWrapper::max( m_periodStats.m_periodStart ); |
There was a problem hiding this comment.
How can they be different over ranks?
Isn't SourceFluxStatistics::gatherTimeStepStats() called synchonously?
There was a problem hiding this comment.
I think for mpi ranks without cells in the region, there is a bypass at CompositionalMultiphaseBase.cpp:1813-1816 - haven't checked single phase, but probably the same thing. This means some of the processes don't get to call SourceFluxStatistics::gatherTimeStepStats() and lag behind. Hence the need for a max.
There was a problem hiding this comment.
Good catch!
Same thing, I would add a short comment to explain why this max() call.
MelReyCG
left a comment
MelReyCG
left a comment
There was a problem hiding this comment.
just a few comments requests
|
I was thinking, if a process is not involved in this computation, could it be excluded in some way? (if elementcount is 0 as an example) |
2 participants
Fixes: #4010
This PR addresses issues regarding the collection and merge of source flux statistics.
Changes Made