-
Notifications
You must be signed in to change notification settings - Fork 37
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
Enable user boundary conditions for particles #655
Conversation
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.
A few minor documentation nitpicks. Otherwise looks good.
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.
LGTM. None of my comments are blocking.
if (start_index < 0) { | ||
start_index = -1; | ||
break; | ||
} |
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.
Are these changes related to setting the user boundary conditions or is this a separate fix?
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 is a separate fix that came up while I was testing this PR -- for some pools of particles (in particular when using very few particles) one could end up trying to index into an array with start_index
where start_index
was -1
which would segfault.
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.
In addition to the comments I left, how about modifying/adding a test case where this infrastructure is called?
@pgrete I added an |
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.
Thanks for the updates.
I'm already approving pending the potential changes pertaining to the boundary allocation.
PR Summary
Particle swarms are intended to support user-provided boundary conditions, but there was a bit of connectivity missing between the user-side application inputs and the Swarms themselves; specifically, we were checking for correct particle boundaries before user boundaries could be set.
On an earlier attempt at making this change I noticed that
InitUserMeshData
is (I think) not very useful right now because application-specified functions can't access theMesh
variables. In particular, I need to be able to setSwarm
boundary conditions to application-specificuser
values at this point in the mesh setup. I don't useInitUserMeshData
here but I assume we want this change?All this PR does is add the
this
pointer to theMesh
calls toInitUserMeshData
.PR Checklist