-
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
Refactor particle to handle different types #637
Comments
Thanks @kks32. I think the plan on doing that for two-phase first is good, and then refactor the current single-phase MPM slowly. You can assign different tasks to us actually, and I think it would be a good exercise to have someone new like Joel to be part of this refactoring. I do have a few comments/questions on the design:
I will drop comments as they come. I will be thinking about this too. |
Still undecided if we need to derive a new
Same way as we store an unordered_map right now, it's a change in data structure, won't affect the design as a whole. |
I'm not sure if I have understood correctly of all the design. But if we define the functions for different phases in different namespace of |
@tianchiTJ yes, that's correct. I'm still undecided if we need to derive datatypes or we could initialize and transfer Particles without having derived classes, this would require knowing the size. If we can achieve serialization then there is no need for deriving particles, and we can move everything to Particle class and remove the base class altogther. |
Shall we keep the current |
Hi @tianchiTJ what would be the reasoning behind deriving the types, because the only difference would be the |
I just think not change the current structure too much, but you are right, maybe it's not necessary to use the deriving types. Anyway, could we decide it as soon as possible? Or we can not work on the implementation of some important features. @kks32 @bodhinandach |
I'd like to go ahead with this design. Let's put to a vote, if you are happy add a 👍 to this comment, if not 👎 . |
Thanks, @kks32 for the RFC. I some comments (or questions) for the proposed RFC:
Any further thoughts on this?
I think I agree with @tianchiTJ that we need to minimize the changes as much as possible. To my knowledge, the current RFC only fixes the function derivations for common processes in particles. But for special functions, I am still not sure how this will help. |
No, stresses and strains will be there as usual. Nothing is changing in the base particle class, we'll slowly refactor only scalar and vector properties.
Thanks, we'll pass an additional argument of
I can't see why this will be any different from assigning the projection parameter example shown in the RFC. We define a function that accepts a particle pointer and can set and get velocities.
At the moment the idea is to serialize the object data and transfer each particle type separately.
The particle class has a pointer to the cell, so I don't see why not. If we need to store element matrices, it might be better to do something similar to the contact algorithm with a common nodal properties pool.
Particle will be a data container, so any function can be defined that updates the particle properties. So no function will be |
@kks32 @tianchiTJ These are the functions collected from @kks32 some functions listed here are not available in the current develop branch, so if you can't find it, please ignore them as Tianchi and I will organize them according to our PR. @tianchiTJ Can you check and list things that are not available here? Some comments:
General Functions:
Common Properties (Geometric, Kinematics & Material):
Mapping Functions:
Particle Constraints & BC
Fluid:
Mapping Functions (to cell):
TwoPhase:
Mapping Functions (to cell):
Mapping Functions (to node):
|
@bodhinandach Thanks for grouping functions based on type. However, we want to group them by the order of execution in the solver, so we may be able to write bigger functions, which will be generic for all derived types. The exercise was to try to see if we could actually group these functions in an order they appear in the solver, so we don't end up having non-pure virtual functions or specific derived classes having extra public functions. Looking at MPMExplicit and Navier-Stokes solver I can't see a potential set of global particle functions than can then call local private functions in the derived type. Additional context@kks32 would like to avoid derived classes behaving like a superset to the base class or have virtual functions that are not defined in all the derived classes. @bodhinandach suggested modifying this list of functions in different particle types into a superset of functions, which are available in all particle types, such as This discussion and the listing of functions is to check if such a grouping of particle functions is feasible, leaving no virtual functions. The benefit is potentially avoiding virtual functions and has safe overriding. Even if we assume this approach is feasible, hiding away all the functions as private would mean a smaller exposed interface, which will not be easy to extend and add functionality. It is also difficult to write tests because all these functions will be private. Testing would then require calling |
After some more consideration, I am wondering about the possibility to derive particle types, but only keep the common particle functions within the class, and merge all independent/specialized functions to be outside the class. This will help to initialize the classes with the right types, and also in MPI transfer, as the type is known. We derived data of individual Particle types but keep specialized particle functions separately. Maybe the particle functions may have to stay in the same namespace. |
This design was 30% slower than private variables so closing this RFC. See #654 for more details |
Summary
Refactor
Particle
data to have the generic functions with the ability to fetch and modify generic scalar and vector properties, map, and interpolate these properties to nodes. Add a separate implementation functions to operate on these properties of the particle. These functions could be grouped under the namespace of solid particle functions, fluid-particle function, and mixed particle functions. The particle agnostic design of functions would allow for mix-and-matching different functions in the Solver class, without having to deal with multiple-inheritance or repetition.Motivation
Refactoring and separating the Particle data from the implementation of algorithm enables handling different particle data types, without having to have a lot of virtual functions in the ParticleBase and other subsequent derived particles. Addresses the comments raised in RFCs #633 and #634. For example, deriving
FluidParticle
fromParticle
would result in extra data needed forFluidParticle
, which is not needed in the case ofParticle
conversely,FluidParticle
won't need certainParticle
data. However, the important consideration is that the functions of aFluidParticle
s have to be virtualized in the abstractParticleBase
and every time a new class is derived, more abstract functions have to be added to the base and other existing derived classes. These functions, which are not useful, must, therefore, throw an error. The Abstract Factory design with inconsistent derived functions is inherently prone to errors when a developer fails to add functions to all the derived classes. This RFC proposes an alternate way of deriving particle data and keeping the implementation separate and reusable. Separation of implementation allows for the composition of different functions in the Solver class.Design Detail
Particle class has a generic ordered map of scalar and vector properties. An open addressing scheme will be used to store the map properties, that the data stored in here will remain continuous in memory. These properties can be accessed using
scalar_property
andvector_property
.Particle
types will still control the initialization of these different properties, i.e., derived classes will be used to create new types of Particles, but all these classes will have a consistent functional interface. This avoids the problem of too many virtual functions. TheParticleBase
will be altered to have the following. A consistent interface allows transferring particles across MPI ranks.Scalar and vector properties can be set and returned using generic functions.
Nodes will also have a similar property pool structure, which will allow us to write generic map and interpolation functions.
This property pool structure provides a generic interface to assign and get properties. We can now define a collection of functions under a namespace, such as
particle::fluid
to define functions that operate on fluid properties. The initialization of the property pool for the Particle will be done by the derivedParticle
class. Each particle type implementation will have access to the property pool data.In the custom solver for the fluid particle, we'll have:
To operate on a specific set of particle types, we could use
iterate_over_particles_with_predicate
functionality that can filter particles with a particular type.With this approach, a solver class can build on different types of Particle data and functions pooled from multiple namespaces.
The proposal is to start moving new
Particle
types to use this design, and the existing solid particle will be refactored in phases.Drawbacks
The implementation operating on the derived data types is separated from the actual data, which is good in some ways, but also it might be seen as breaking encapsulation. Considering the fact that if something owns a
Particle
type, then it should be able to modify the data in theParticle
. This separation of data and implementation may give some developers a false feeling that their implementation could work on allParticle
types, however, as soon as the code is run or compiled with debug mode, it will be known that the data type is unsupported and cannot be used. The phase refactoring of Solid Particles will break a lot of existing code, however, it will minimize the number ofupdate_nodes
andinterpolate_from_nodes
design.Rationale and Alternatives
Allows for future composition in the solver class with multiple Particle types, multiple
particle::fluid
functions can be defined and used in different solver classes.Avoids complex inheritance schemes, for example,
TwoPhaseParticle
doesn't have to redefine functions influid
orsolid
when derived from only one type.FactoryDesign (Abstract Class): This results in too many virtual functions, an addition of a new function in one of the derived classes affects all existing
Particle
classes.BridgeDesign: Requires a class for the implementation that takes the
ParticleData
, same as Abstract class, results in virtual functions pushed to implementation.std::variant: Good alternative to removing abstract base classes. However, may not be a good alternative, as we still need to separate the functions from the data. Also, different template instantiation is not possible.
Super functions: Use more generic super functions which are public member functions and specialized functions for each derived type are private member functions. This design avoids non-pure virtual functions, however, limits the interface functions so it is not flexible and writing test functions are difficult.
Consistent interface cannot be achieved on different particles and will have to refactor existing classes every time a new function is added.
Consistent interface helps with MPI data transfer.
Prior Art
Most large codes avoid a lot of derived classes and Abstract class design with inconsistent interfaces across derived classes.
Unresolved questions
New Particle types will follow this new design, while the existing solid particle has to be slowly refactored.
The refactoring of solver class to make it more composable will be discussed in a separate RFC.
How to handle MPI data transfer with an arbitrary number of particle data types? Since the
ParticleBase
will be derived for each type of Particle, we will have enough information to transfer particles. Suggest creating serialization and deserialization schemes. Additionally, since the number of activeParticle
types are going to be a handful at most, we can iterate through different types when doing MPI halo exchange.Still undecided if we need to derive a new
Particle
for each type, so the advantage of this method is that the derivedParticle
type knows its variables and is easy for instantiation. However, it creates additional vtables. On the other hand, if we move the variable instantiation outside of theParticle
, we don't have to derive new types, but initialization and MPI transfer may become harder?Changelog
[Edit #1]: Update
iterate_over_mesh
function and formatting.[Edit #2]: Add std::variant as alternatives and prior art
[Edit #3]: Unresolved question on deriving new Particle types
[Edit #4]: Discussion on superfunctions as an alternative
The text was updated successfully, but these errors were encountered: