-
Notifications
You must be signed in to change notification settings - Fork 84
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
[Solver] Two-Phase One-Point Explicit #680
base: develop
Are you sure you want to change the base?
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.
Partial review: Could we move the fluid function implementations in mesh to a separate "mesh_fluid.tcc" file so it's easier to manage?
|
||
//! Map cell volume to the nodes | ||
//! \param[in] phase to map volume | ||
void map_cell_volume_to_nodes(unsigned phase); |
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.
Would this affect GIMP and other non-local algorithms?
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.
Hmm... That's a good point, but I don't think the free-surface detection can be used for non-local MPM yet in general, so I am expecting this function won't work well too for non-local MPM.
@kks32 I take some of the functions out to |
…/mpm into solver/two-phase-explicit
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.
Partial review: Review contains comments for mesh amd mesh_mutliphase files.
//! Map cell volume to nodes | ||
template <unsigned Tdim> | ||
void mpm::Cell<Tdim>::map_cell_volume_to_nodes(unsigned phase) { | ||
if (this->status()) { |
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.
Why are we doing this only for cells with particles? The function looks generic so adding this for only cells with particles looks strange. Could we instead pass it as a function argument?
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.
We only do this for cells with particles as otherwise, the nodes_->update_volume()
will be called as many numbers as the MPI thread. Therefore, without checking the status()
, we won't have the free_surface detection working in parallel.
|
||
//! Check internal cell | ||
for (const auto neighbour_cell_id : (*citr)->neighbours()) { | ||
#if USE_MPI |
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.
Why not use the same functions? solving_status
?
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 that case, we need to assign solving_status
for a single MPI threat too, which is just a copy of status
include/mesh_multiphase.tcc
Outdated
// Assign free surface nodes | ||
if (!common_node_id.empty()) { | ||
for (const auto common_id : common_node_id) { | ||
map_nodes_[common_id]->assign_free_surface(true); |
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 needs to be reduced across MPI tasks
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.
Good point! I think, since we did the solving_status
all_reduce
prior to this call, we don't have to do so.
bool status = true; | ||
|
||
try { | ||
if (!particles_.size()) |
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.
Use assert to check this condition
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.
Actually, we just followed the format of other assign_particles_xxx
functions. If we change this one, will that be better to change all other functions in mesh.tcc
?
mpm::ParticlePhase::Liquid); | ||
} | ||
} catch (std::exception& exception) { | ||
console_->error("{} #{}: {}\n", __FILE__, __LINE__, exception.what()); |
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 we OK if this fails and we can continue the calculation with default values?
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 think that would be better if we stop the simulation.
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 you make sure all return functions are marked const
- Partial review
include/node.h
Outdated
@@ -245,6 +260,21 @@ class Node : public NodeBase<Tdim> { | |||
//! Set ghost id | |||
void ghost_id(Index gid) override { ghost_id_ = gid; } | |||
|
|||
//! Return real density at a given node for a given phase |
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.
What's a real
density? is it the interpolated density of the particles?
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.
Please make sure the return statements are all const
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.
Yes, it is the interpolated particle density, but not from mass_density
. It is computed by the division of mass_
/volume_
inside nodes. Those two variables are the interpolated variables from particles. I think I will change the detail to "approximated" density instead of "real" density.
include/node_twophase.tcc
Outdated
|
||
// Apply velocity constraints, which also sets acceleration to 0, | ||
// when velocity is set. | ||
this->apply_velocity_constraints(); |
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.
Does this apply velocity constraints to both solid and fluid phases?
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.
Yes, according to this:
Lines 362 to 367 in 689d9fb
// Direction value in the constraint (0, Dim * Nphases) | |
const unsigned dir = constraint.first; | |
// Direction: dir % Tdim (modulus) | |
const auto direction = static_cast<unsigned>(dir % Tdim); | |
// Phase: Integer value of division (dir / Tdim) | |
const auto phase = static_cast<unsigned>(dir / Tdim); |
const mpm::dense_map& state_vars, | ||
const std::shared_ptr<mpm::Material<Tdim>>& material, | ||
unsigned phase = mpm::ParticlePhase::Solid) override; | ||
PODParticle& particle, |
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 we pass it as a const&
?
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 checked this one, it is not possible as we reinterpret_cast
it inside the derived function:
mpm/include/particles/particle_twophase.tcc
Lines 167 to 170 in 689d9fb
bool mpm::TwoPhaseParticle<Tdim>::initialise_particle(PODParticle& particle) { | |
// Initialise solid phase | |
bool status = mpm::Particle<Tdim>::initialise_particle(particle); | |
auto twophase_particle = reinterpret_cast<PODParticleTwoPhase*>(&particle); |
mpm/include/particles/particle_twophase.tcc
Lines 199 to 204 in 689d9fb
template <unsigned Tdim> | |
bool mpm::TwoPhaseParticle<Tdim>::initialise_particle( | |
PODParticle& particle, | |
const std::vector<std::shared_ptr<mpm::Material<Tdim>>>& materials) { | |
auto twophase_particle = reinterpret_cast<PODParticleTwoPhase*>(&particle); | |
bool status = this->initialise_particle(*twophase_particle); |
//! Free surface | ||
bool free_surface_{false}; | ||
//! Free surface | ||
Eigen::Matrix<double, Tdim, 1> normal_; |
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.
Specifically a free_surface_normal_
?
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.
Do I need to change the name of the assign
and return
functions too?
@@ -284,6 +290,16 @@ bool mpm::Particle<Tdim>::assign_material_state_vars( | |||
return status; | |||
} | |||
|
|||
//! Assign a state variable | |||
template <unsigned Tdim> | |||
void mpm::Particle<Tdim>::assign_state_variable(const std::string& var, |
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.
Why do we need this function to assign a specific state variable?
We should check this in assert
, and go ahead without throwing an error. No reason to throw, if we can't recover.
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 think we need this to assign_pressure
or pore_pressure
, just to make it generalize as when we call pressure()
, we actually calling state_variable("pressure")
.
I agree with you to make it in assert
instead of throwing an error.
//! Update porosity | ||
//! \param[in] dt Analysis time step | ||
virtual void update_porosity(double dt) { | ||
throw std::runtime_error( |
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.
Why do we have to do this? I would keep this pure virtual and throw error elsewhere?
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.
That means we need to populate the particle.h
with all these functions. Are you okay with that?
include/solvers/mpm_base.tcc
Outdated
// Pore pressure constraint phase indice | ||
unsigned constraint_phase = constraints["phase_id"]; | ||
|
||
if (constraint_phase >= Tnphases) |
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.
We can't check phases in MPM class, we should check it in the nodes
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.
Super cool. I removed this check. It will be automatically called as the constraints_->assign_nodal_pressure_constraints()
function is called. In short, they will reach this assertion.
Lines 178 to 186 in 689d9fb
//! Assign pressure constraint | |
template <unsigned Tdim, unsigned Tdof, unsigned Tnphases> | |
bool mpm::Node<Tdim, Tdof, Tnphases>::assign_pressure_constraint( | |
const unsigned phase, const double pressure, | |
const std::shared_ptr<FunctionBase>& function) { | |
bool status = true; | |
try { | |
// Constrain directions can take values between 0 and Tnphases | |
assert(phase < Tnphases * 2); |
@kks32 Thanks for your time reviewing this PR. It looks much better now. I partially modified the PR according to your comments. Can you spend some time to check it again? |
ping @kks32, any further issues with this branch. Other than dynamic load balancing and resume feature in MPI, I am good. |
@kks32 Some missing features which are not available in MPI while running 2Phase:
|
Describe the PR
This PR is to implement the two-phase one-layer explicit solver for fully saturated soil simulation. One particle includes both solid phase and liquid phase, which are defined by the corresponding volume fractions (
n_s
andn_w
). The theory of porous media is adopted to set up the governing equations (momentum balance and mass balance). The pore pressure is updated based on the strain of the solid skeleton, and a weakly compressible fluid assumption is assumed to solve the fluid and mixture balance equations. All implementations work coherently with the current MPI and OpenMP parallelization features.New implementation
TwoPhase Particle
ParticleTwoPhase
derived fromParticle
, which include all necessary functions for two-phase computation and new variables for two-phase (etc. pore pressure, porosity)ParticleTwoPhase
(also includes refactoring of base hdf5 class)MPMExplicitTwoPhase
MPMExplicitTwoPhase
derived fromMPMBase
, which includes the main loop for two-phase computation.MPMBase
to read the new types of input files for two-phase.TwoPhase functions for nodes
Node
class directly (seenode_multiphase.tcc
)Free surface detection functions in mesh
Mesh
class (seemesh_multiphase.tcc
)Unit tests and benchmarks
All new implementations are validated by corresponding test functions, and the unit-test for two-phase computation is also included. Related 2D and 3D one-dimensional consolidation tests can be found here: cb-geo/mpm-benchmarks#19.