-
Notifications
You must be signed in to change notification settings - Fork 90
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 some issues with parallel BCs #2540
Conversation
Not checking can lead to seqfaults.
The addition / substraction does not preserve parallel fields thus the old code does not work. I am not aware of anybody using that with FCI, so dropping it.
src/field/field3d.cxx
Outdated
@@ -514,17 +514,12 @@ void Field3D::applyParallelBoundary() { | |||
TRACE("Field3D::applyParallelBoundary()"); | |||
|
|||
checkData(*this); | |||
ASSERT1(!yup_fields.empty()); |
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.
Is it worth checking ydown_fields
too? They should always be initialised together, but you never know...
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.
hasParallelSlices()
does this, that might also be more explicit:
ASSERT1(!yup_fields.empty()); | |
ASSERT1(hasParallelSlices()); |
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.
I was looking for this function, but didn't search in the header file 🙈
* Is easier to understand * Allows to have checks in a single place * I have never seen a bug where the slices where not in sync, so just checking one field for the optimized case should be sufficient.
It looks like this is likely a bug in Github Actions |
#if CHECK > 2 | ||
if (yup_fields.size() != ydown_fields.size()) { | ||
throw BoutException( | ||
"Field3D::splitParallelSlices: forward/backward parallel slices not in sync.\n" | ||
" This is an internal library error"); | ||
} | ||
#endif | ||
#if CHECK | ||
return !yup_fields.empty() and !ydown_fields.empty(); | ||
#else | ||
return !yup_fields.empty(); | ||
#endif |
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.
Moving the check that the sizes equal here is good idea 👍
The optimisation if CHECK
is off feels a little excessive, but I don't think it hurts 😄
No description provided.