-
Notifications
You must be signed in to change notification settings - Fork 60
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
Dem volume solid objects #1169
Dem volume solid objects #1169
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.
Some general comments before a start the review:
- I'm not sure the title of the PR is representative of the content for the current work. For me this is more an introduction to 3D (volume) solid object than a moving manifold thing.
- I would add the comment from Miscellaneous to the description. In this case especially, this is an important info since the work is not at a full working state.
- I saw you modified the application tests parameter files. Did you check the example files? (EDIT: just saw the failed test about it, so I guess you flaged it x))
- Since the volume solid objects are not fully working (as it is for 2d solid object), I would put a huge warning as soon in the "solid_object_parameters.h" to not use it yet.
- About the PR description, please give the name of the old parameters and the new ones
I will look at the code content probably tomorrow :)
@@ -10,45 +10,37 @@ Floating meshes (solid objects) are finite (limited) auxiliary objects that can | |||
.. note:: | |||
At the moment, solid objects (floating meshes) in Lethe have to be defined using triangular (simplex) meshes. Only triangular 2D meshes of 3D surfaces in 3D DEM simulations are presently supported. Quadrilateral 2D meshes of 3D surfaces and 1D mesh of 2D surfaces are not supported at the moment. | |||
|
|||
This subsection explains the solid objects (floating meshes) information. First of all, the ``number of solids`` is specified. Then, for each solid object, we need a ``mesh`` subsection. In these subsections, the ``type``, ``initial refinement``, and other properties of the objects are defined. Note that ``simplex`` must be ``true`` for all the objects since the solid objects are defined using simplex meshes. For more information on mesh subsection, visit `CFD mesh <https://chaos-polymtl.github.io/lethe/documentation/parameters/cfd/mesh.html>`_ | |||
This subsection explains the solid objects (floating meshes) information. First of all, the ``number of solids`` is specified. Then, for each solid object, we need a ``mesh`` subsection. In these subsections, the ``type``, ``initial refinement``, and other properties of the objects are defined. Note that currently, only ``two dimensions`` solid object are usable. The ``simplex`` parameter must be set to ``true`` for all the objects since 2D solid objects are defined using simplex meshes. For more information on mesh subsection, visit `CFD mesh <https://chaos-polymtl.github.io/lethe/documentation/parameters/cfd/mesh.html>`_ |
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 suggest we agree on the name of the floating mesh/solid object? what do you thing?
The page is "Floating Mesh (Solid Objects)" but here it says "...the solid objects (floating meshes)..." x)
We should choose one for 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.
I would use solid object exclusively. If we both agree on this, I could do a PR to fix this.
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.
It will needs many in-code changes tho. Floating wall also needs to change.
We can agree on Solid object and virtual wall :D ?
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.
Yup I agree with this. Virtual wall especially is a very good nomenclature.
@@ -59,7 +51,7 @@ This subsection explains the solid objects (floating meshes) information. First | |||
|
|||
* In the subsection ``angular velocity``, we define the angular velocity of the floating mesh. | |||
|
|||
* In the subsection ``center of rotation``, we define the center of rotation of the solid object (in the case of rotational motion). | |||
* The ``center of rotation`` defines the center of rotation of the solid object (in the case of rotational motion). |
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'm not sure about it, but maybe the center of rotation should also be a function.
What happens when your mesh moves in x but the rotation axis is in the xy plan? (let say reproducing a rolling cylinder on a surface)
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.
The center of rotation
is being translated following the translational velocity
function, so it follows the solid object.
It is like if you were adding a vertex on the solid. This new "vertex" is being translated like every other vertex.
This is a non issue. I can explain it more in detail with you this week.
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 I get it now, I just did not know that the point was translated
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.
Yeah otherwise it's a pain in the ass to code
It could be good to add a note that says that the center of rotation is translated with the translational velocity
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 could also change the way we handle rotation. Solid surface have a tendency to deform when put into rotation.
We could define the object in its frame of reference and add the rotation matrix to the object.
Instead of translating every vertex individually, we could translate the frame of reference and to the whole rotation from the start. I don't know what would be the computational cost of this procedure. TBD
// Cast to std::shared_ptr<Function<dim>> after parameter declaration | ||
translational_velocity = translational_velocity_parsed; | ||
angular_velocity = angular_velocity_parsed; |
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 don't understand why this step?
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.
translational_velocity and angular_velocity used to be Functions::ParsedFunction<dim>
. The only way to define a ParsedFunction is to parse a prm file, which is not really nice when making a test. This is why they need to be of type std::shared_ptr<Function<dim>>
The Function
class doesn't have the declare_parameter
and parse_parameter
functions ( they are only implemented in the ParsedFunction class)
In essence, if I don't use those two temporary variables, I'm not able to use the declare_parameter
and parse_parameter
functions or I'm not able to define a random function for my test.
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.
Oh ok, got it
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.
Yeah that's a bit convoluted, but I'm not sure there's an easier solution than this other than to do a dynamic typechecking there.
I think this is ok though, Just comment it so we know why you are doing this. LIke add a comment that explains why you are declaring these shared_ptr to not use them afterwards. If Audrey asked the question, it means from the comments it was not clear.
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 suggest we start using the name solid_surfaces and solid_volumes to refer to <2,3> and <3,3> solid objects. I feel this would make our life much, much easier from now on to clearly define a nomenclature.
I would improve some of your comments to make them clearer. I left a few comments there and there regarding this
@@ -10,45 +10,37 @@ Floating meshes (solid objects) are finite (limited) auxiliary objects that can | |||
.. note:: | |||
At the moment, solid objects (floating meshes) in Lethe have to be defined using triangular (simplex) meshes. Only triangular 2D meshes of 3D surfaces in 3D DEM simulations are presently supported. Quadrilateral 2D meshes of 3D surfaces and 1D mesh of 2D surfaces are not supported at the moment. | |||
|
|||
This subsection explains the solid objects (floating meshes) information. First of all, the ``number of solids`` is specified. Then, for each solid object, we need a ``mesh`` subsection. In these subsections, the ``type``, ``initial refinement``, and other properties of the objects are defined. Note that ``simplex`` must be ``true`` for all the objects since the solid objects are defined using simplex meshes. For more information on mesh subsection, visit `CFD mesh <https://chaos-polymtl.github.io/lethe/documentation/parameters/cfd/mesh.html>`_ | |||
This subsection explains the solid objects (floating meshes) information. First of all, the ``number of solids`` is specified. Then, for each solid object, we need a ``mesh`` subsection. In these subsections, the ``type``, ``initial refinement``, and other properties of the objects are defined. Note that currently, only ``two dimensions`` solid object are usable. The ``simplex`` parameter must be set to ``true`` for all the objects since 2D solid objects are defined using simplex meshes. For more information on mesh subsection, visit `CFD mesh <https://chaos-polymtl.github.io/lethe/documentation/parameters/cfd/mesh.html>`_ |
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 subsection explains the solid objects (floating meshes) information. First of all, the ``number of solids`` is specified. Then, for each solid object, we need a ``mesh`` subsection. In these subsections, the ``type``, ``initial refinement``, and other properties of the objects are defined. Note that currently, only ``two dimensions`` solid object are usable. The ``simplex`` parameter must be set to ``true`` for all the objects since 2D solid objects are defined using simplex meshes. For more information on mesh subsection, visit `CFD mesh <https://chaos-polymtl.github.io/lethe/documentation/parameters/cfd/mesh.html>`_ | |
This subsection explains the solid objects (floating meshes) information. First of all, the ``number of solids`` is specified. Then, for each solid object, we need a ``mesh`` subsection. In these subsections, the ``type``, ``initial refinement``, and other properties of the objects are defined. Note that currently, only two dimensional solid objects, meaning surfaces, are usable. The ``simplex`` parameter must be set to ``true`` for all the objects since 2D solid objects are defined using simplex meshes. For more information on mesh subsection, visit `CFD mesh <https://chaos-polymtl.github.io/lethe/documentation/parameters/cfd/mesh.html>`_ |
I would try to add a mention that this is a dim=2 spacedim=3 triangulation. This would make it clear for anyone familiar with deal.II
@@ -59,7 +51,7 @@ This subsection explains the solid objects (floating meshes) information. First | |||
|
|||
* In the subsection ``angular velocity``, we define the angular velocity of the floating mesh. | |||
|
|||
* In the subsection ``center of rotation``, we define the center of rotation of the solid object (in the case of rotational motion). | |||
* The ``center of rotation`` defines the center of rotation of the solid object (in the case of rotational motion). |
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.
Yeah otherwise it's a pain in the ass to code
It could be good to add a note that says that the center of rotation is translated with the translational velocity
source/dem/dem.cc
Outdated
@@ -1556,7 +1600,7 @@ DEMSolver<dim>::solve() | |||
triangulation, | |||
particle_handler, | |||
insertion_object, | |||
solids, | |||
solids_2d, |
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.
maybe solid_surfaces and solide_volumes would be the better name here.
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 like it (surface/volume)!
include/dem/dem.h
Outdated
std::vector<std::shared_ptr<SerialSolid<dim - 1, dim>>> solids_2d; | ||
std::vector<std::shared_ptr<SerialSolid<dim, dim>>> solids_3d; |
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.
maybe solid_surfaces and solid_volumes?
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.
Forget about my comment earlier. This answer my question. :)
source/core/serial_solid.cc
Outdated
translational_velocity = | ||
std::make_shared<Functions::ParsedFunction<spacedim>>(spacedim); | ||
angular_velocity = | ||
std::make_shared<Functions::ParsedFunction<spacedim>>(spacedim); | ||
translational_velocity = param->translational_velocity; | ||
angular_velocity = param->angular_velocity; |
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.
If those are shared point (translational velocity and angular velocity) you don't need to allocate them. You just set them to be equal to the other ones because they should point to the same functions
Otherwise you can call the copy constructor, but there is no need to allocate to then erase the pointer.
source/core/serial_solid.cc
Outdated
simulation_control->get_output_name() + ".solid_object." + | ||
Utilities::int_to_string(dim, 1) + "." + Utilities::int_to_string(id, 2); |
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 would really start calling them solid surfaces and solid volumes from now on.
end | ||
|
||
set center of rotation = 0., 0., 0. | ||
set output solid object = 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.
The indentation should be fixed here
|
||
// Functions | ||
std::vector<double> translational_vector = {1., 0., 0.}; | ||
std::vector<double> angular_vector = {-0.39269908169, 0., 0.}; // 0.125 * pi |
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.
std::vector<double> angular_vector = {-0.39269908169, 0., 0.}; // 0.125 * pi | |
std::vector<double> angular_vector = {-0.39269908169, 0., 0.}; // - 0.125 * pi |
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.
First batch for the documentation.
Also please don't forget to gives the name of the old vs new parameters in the PR doc and the name of the new test :)
Also, Just verify for other "floating mesh" mention in the doc of solid objects
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.
Nice :) It's sure it is not finished yet, so there are a few places that will need the distinction between surface and volume (like has_solid_objects), but this is fine to me.
Maybe I would open an issue about the fact that there are solid volume, but it needs proper implementation for contacts.
@@ -141,19 +141,19 @@ class ParticleWallBroadSearch | |||
|
|||
void | |||
particle_floating_mesh_contact_search( |
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.
The name of the function is still particle_floating_mesh_contact_search()
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 catch
Just saw that there is a test called floating_mesh.prm if you want to change it |
CHANGELOG.md
Outdated
|
||
### Changed | ||
|
||
- MINOR A new subsection ``solid surfaces`` need to be used when defining solid objects in the parameter file. For more information, see the solid object documentation. [#1169](https://github.com/chaos-polymtl/lethe/pull/1169) |
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.
- MINOR A new subsection ``solid surfaces`` need to be used when defining solid objects in the parameter file. For more information, see the solid object documentation. [#1169](https://github.com/chaos-polymtl/lethe/pull/1169) | |
- MINOR A new subsection ``solid surfaces`` needs to be used when defining solid objects in the parameter file. For more information, see the solid object documentation. [#1169](https://github.com/chaos-polymtl/lethe/pull/1169) |
doc/source/examples/dem/granular-dam-break/granular-dam-break.rst
Outdated
Show resolved
Hide resolved
3df641c
to
a4042ae
Compare
a4042ae
to
8c4555f
Compare
568a2b9
to
4279126
Compare
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.
The content of the Miscellaneous section is actually matters of the Documentation in the PR description.
Also, is there a reason you don't resolve the comments in the PR once the topic is discussed or modification has been applied? When I review I think that it's not done when it's still there, and this is why I do not rereview or approve. So I just want to know if there is a specific reason and next time I should just change the way I understand it :)
Otherwise, all good for me!
I don't get what you mean about the Miscellaneous section . I did add the changed parameter, do I need to do something else? About the resolve thing, the comments remaining were not modification requests, but more like questions. I like to keep those open because sometime I'm not the one answering those question (like the one Bruno gave about the SerialSolid thing), or someone else would like to comment over my anwser. |
This is not reviewed and ready to merge until @oguevremont has approved the PR.... then I'll review/merge |
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.
1-2 last things, seems good to go.
I presume the virtual wall (instead of floating wall) will be made in a follow-up PR?
Insertion information including the dimensions of the insertion box, insertion frequency, etc. are defined in the ``insertion info`` section. | ||
The``mesh`` section defines the simulation triangulation and refinements. | ||
In the ``DEM boundary conditions`` section, boundaries can be defined as outlets or they can get periodicity, rotation or translation. | ||
In the ``floating walls`` section, hyperplanes can be added to the simulation domain. |
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.
These were finally not renamed to virtual walls in this PR?
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.
No, not in this PR.
Solid Objects | ||
============================= | ||
|
||
Solid objects are finite auxiliary objects that can be stationary or in motion. Rotating impellers, sliding surfaces, and finite stoppers are examples of solid objects. The main differences between them and floating walls are: |
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.
Solid objects are finite auxiliary objects that can be stationary or in motion. Rotating impellers, sliding surfaces, and finite stoppers are examples of solid objects. The main differences between them and floating walls are: | |
Solid objects are finite auxiliary objects that can be stationary or in motion. Rotating impellers, sliding surfaces, and finite stoppers are examples of solid objects. The main differences between them and floating walls are: |
Again the change to virtual wall was not made here?
Yes, that what I had in mind regarding the virtual walls. |
Co-authored-by: Bruno Blais <[email protected]>
guevremont comment Co-authored-by: Olivier Guévremont <[email protected]>
Description Solid objects are currently only using simplex. This can be problematic since double contacts can occur at the junction of two triangles. This PR aims at introducing solid objects built from volume elements, which could solve this problem. The contact between particle and volume 3D object is not implemented yet. Only in the creation and the displacement. Testing A new test has been implemented. (volume_solid_object_displacement.cc) Documentation Old simulation parameters have been renamed. The center of rotation was being defined by 3 parameters. Subsection center of rotation and its three parameters (x, y and z) have been replaced by one parameter named center of rotation using a list of doubles. A new subsection solid surfaces need to be used when defining solid object in the parameter file. Co-authored-by: Audrey Collard-Daigneault <[email protected]> Co-authored-by: Olivier Guévremont <[email protected]> Co-authored-by: Bruno Blais <[email protected]>
Description Solid objects are currently only using simplex. This can be problematic since double contacts can occur at the junction of two triangles. This PR aims at introducing solid objects built from volume elements, which could solve this problem. The contact between particle and volume 3D object is not implemented yet. Only in the creation and the displacement. Testing A new test has been implemented. (volume_solid_object_displacement.cc) Documentation Old simulation parameters have been renamed. The center of rotation was being defined by 3 parameters. Subsection center of rotation and its three parameters (x, y and z) have been replaced by one parameter named center of rotation using a list of doubles. A new subsection solid surfaces need to be used when defining solid object in the parameter file. Co-authored-by: Audrey Collard-Daigneault <[email protected]> Co-authored-by: Olivier Guévremont <[email protected]> Co-authored-by: Bruno Blais <[email protected]> Former-commit-id: 4207163
Description Solid objects are currently only using simplex. This can be problematic since double contacts can occur at the junction of two triangles. This PR aims at introducing solid objects built from volume elements, which could solve this problem. The contact between particle and volume 3D object is not implemented yet. Only in the creation and the displacement. Testing A new test has been implemented. (volume_solid_object_displacement.cc) Documentation Old simulation parameters have been renamed. The center of rotation was being defined by 3 parameters. Subsection center of rotation and its three parameters (x, y and z) have been replaced by one parameter named center of rotation using a list of doubles. A new subsection solid surfaces need to be used when defining solid object in the parameter file. Co-authored-by: Audrey Collard-Daigneault <[email protected]> Co-authored-by: Olivier Guévremont <[email protected]> Co-authored-by: Bruno Blais <[email protected]> Former-commit-id: 4207163
Description
Solid objects are currently only using simplex. This can be problematic since double contacts can occur at the junction of two triangles. This PR aims at introducing solid objects built from volume elements, which could solve this problem. The contact between particle and volume 3D object is not implemented yet. Only in the creation and the displacement.
Testing
A new test has been implemented. (volume_solid_object_displacement.cc)
Documentation
Old simulation parameters have been renamed. The center of rotation was being defined by 3 parameters.
Subsection
center of rotation
and its three parameters (x
,y
andz
) have been replaced by one parameter namedcenter of rotation
using a list of doubles.A new subsection
solid surfaces
need to be used when defining solid object in the parameter file.Miscellaneous (will be removed when merged)
Checklist (will be removed when merged)
See this page for more information about the pull request process.
Code related list:
Pull request related list: