Periodic boundaries #361
Periodic boundaries #361
Conversation
ryan-bournes
added
bdm-core
|
I've updated the clamp function so it uses the standard version and removed the in-built clamp function. This clamp function was only used in the Neumann and Periodic boundary calculations. Seems like everything works fine. Is there any other changes which need to be made? |
src/core/diffusion/euler_grid.cc
Outdated
| // Clamp to avoid out of bounds access. Clamped values are initialized | ||
| // to a wrong value but will be overwritten by the boundary condition | ||
| // evaluation. All other values are correct. | ||
| real_t left{c1_[std::clamp(l, size_t{0}, num_boxes - 1)]}; |
There was a problem hiding this comment.
Is this necessary? The indices should never be out of bounds. I think this piece of code makes it a bit harder to understand. can we drop all these real_t variables and simply use c1_[l] for instance?
Same comment goes for this piece of code above in Neumann, can we double check if we can get rid of it?
There was a problem hiding this comment.
I will be honest I am not sure the point of it as well. I copied the Neumann boundary code as a base for creating the Periodic boundaries and I figured it had a reason to be there so I kept it in. But we can try taking it out and seeing if any of the unit tests fail.
|
|
|
I've added the explantions for the param file and diffusion.md file. Removing the clamp functions seems to break the unit tests on macOS, so i've just reverted the changes and kept the clamp functions in. |
Remove in-built clamp function and replace with standard clamp function coming with c++17. The standard function requires all input values to be the exact same type, so all input values are set to size_t.
TobiasDuswald
force-pushed
the
periodic_boundaries
branch
from
677b89a to
bbe5789
Compare
|
@ryan-bournes see bbe5789 regarding my comment earlier. |
|
|
Alright, from my side this is ready! Thanks @ryan-bournes ! Green light from your side as well or are you still planning to push some changes? |
|
Thank you @TobiasDuswald and @nicogno ! In terms of the periodic boundaries, this functionality can be pushed to the main branch. I have mostly everything ready for the complex boundary implementation, but I would rather add these to the master branch one at a time, so I will open a new pull request for the complex boundaries. Do you think this is a good idea? |
This is the pull request for adding periodic boundaries to the diffusion grid. This is simply an extra function on top of Neumann, Dirichlet, open and closed boundaries that the user can choose from. This also includes a unit test to confirm the boundary conditions work.
Periodic boundaries allows a diffusive substance to move from one boundary to another. This feature is the same as the taurus boundaries for agents, except it is for diffusion. An illustration can be seen below:
When calculating diffusion in u0, it will look at the concentration in u1 and uN-1 to determine the change. If the concentration in uN-1 is lower/higher than u0 then concentration will be transferred from/to u0. Likewise when calculating diffusion in uN-1, it will look at uN-2 and u0, and determine the change in the same way. This transfer through the boundary does not modify the total concentration in the system: it only affects how the concentration can move through the simulation space. Many biological systems simulated with BioDynaMo represent small slices of the much larger system. Periodic boundaries promotes the idea of concentration coming from "other parts" of the biological system. There is a subtle but important difference between a diffusive substance colliding with a large concentration of substances vs colliding with a solid wall.
@TobiasDuswald and @nicogno since you two implemented the Neumann and Dirichlet boundaries, I figured you would be the right people to review this update. Feel free to take a look at this request and potentially do any stress testing when you have time.