Dem particle wall sliding velocity #896
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
OGaboriault
reviewed
Oct 1, 2023
| @@ -28,6 +28,9 @@ In this subsection, the boundary conditions of the DEM simulation are defined. F | |||
| set rotational vector x = 1 | |||
| set rotational vector y = 0 | |||
| set rotational vector z = 0 | |||
There was a problem hiding this comment.
Shouldn't we also just change the way we define the rotation the rotation vector in the to be the same as the point on rotation vector. I find the new way more elegant.
There was a problem hiding this comment.
The new way is more elegant, but i want to introduce the change progressively. First fix the bug, then refactor the parameters in another PR. I would like to remove all of the x, y , z parameters as individual and always use lists, but I would rather do this in another PR
| vector_to_rotating_axis = | ||
| vector_to_rotating_axis - | ||
| (vector_to_rotating_axis * this->boundary_rotational_vector[boundary_id]) * | ||
| this->boundary_rotational_vector[boundary_id] / |
There was a problem hiding this comment.
this->boundary_rotational_vector is not already a unit vector?
There was a problem hiding this comment.
we are using the double boundary_rotational_speed, so technically boundary_rotational_vector should be a unit vector. We should add a step when creating that container to make sure that we are storing unit vectors in it. This would save the extra division (and addition at the denominator).
Also, why are you using the norm_square?
There was a problem hiding this comment.
I am using the norm square because we do two scalar product. There is no garantee right now that the vector is a unit vector, (e.g. someone could write 1,1,1 and it should still work). We can refactor this, but this means changing what was the meaning of these parameters. I am all for this, but this should be done in another PR. This PR fixes the problem we had, it does not aim at refactoring the entire parameter structure.
| dem_parameters.boundary_conditions.boundary_rotational_vector, | ||
| grid_radius, | ||
| dem_parameters); | ||
| ParticleWallLinearForce<dim> force_object(dem_parameters); |
There was a problem hiding this comment.
LOL. Ok I see what you mean hahaha. This is really funny.
blaisb
force-pushed
the
dem_particle_wall_sliding_velocity
branch
from
6338bdb to
927717d
Compare
Co-authored-by: OGaboriault <[email protected]>
OGaboriault
approved these changes
Oct 2, 2023
blaisb
added a commit
that referenced
this pull request
Oct 2, 2023
Description of the problem The calculation of the translational velocity on rotating walls was calculated inadequately #896: The velocity of rotating boundary conditions (e.g. boundaries of the mesh) was calculated inadequately in the case where the normal vector of the wall was aligned with the rotation axis. The whole calculation procedure was slightly messed up and only worked for cylinders. Description of the solution This has been fixed and made general by using the vector from the contact point to the center of rotation. A new parameter has been added for this purpose. How Has This Been Tested? All test pass and give adequate results A new application test has been added to test this exact feature Documentation Documentation of the point on the axis has been updated Future changes I will continue to refactor the entire DEM particle-wall aspect to make it cleaner and more streamlined Co-authored-by: OGaboriault <[email protected]>
blaisb
pushed a commit
that referenced
this pull request
Nov 1, 2023
#Description of the problem This is a follow up to Dem particle wall sliding velocity #896 The rotational vector for the rotational boundary condition in the lethe-particles solver is now define with one line in the parameters file. We make sure that the rotational vector is a unit vector. This way, we save the norm_square calculation, the addition and the division when computing the vector_to_rotating_axis variable. (Simply cleaner in my opinion) We throw an error is the vector is defined as the null vector which wasn't the case previously. How Has This Been Tested? All test pass. Throw error works Documentation The set rotation vector line in the documentation and examples were updated. Future changes The translation speed should be define as such in the parameter file Comments None
M-Badri
pushed a commit
to M-Badri/lethe
that referenced
this pull request
Sep 29, 2024
Description of the problem The calculation of the translational velocity on rotating walls was calculated inadequately chaos-polymtl#896: The velocity of rotating boundary conditions (e.g. boundaries of the mesh) was calculated inadequately in the case where the normal vector of the wall was aligned with the rotation axis. The whole calculation procedure was slightly messed up and only worked for cylinders. Description of the solution This has been fixed and made general by using the vector from the contact point to the center of rotation. A new parameter has been added for this purpose. How Has This Been Tested? All test pass and give adequate results A new application test has been added to test this exact feature Documentation Documentation of the point on the axis has been updated Future changes I will continue to refactor the entire DEM particle-wall aspect to make it cleaner and more streamlined Co-authored-by: OGaboriault <[email protected]> Former-commit-id: 82a8a36
M-Badri
pushed a commit
to M-Badri/lethe
that referenced
this pull request
Sep 29, 2024
#Description of the problem This is a follow up to Dem particle wall sliding velocity chaos-polymtl#896 The rotational vector for the rotational boundary condition in the lethe-particles solver is now define with one line in the parameters file. We make sure that the rotational vector is a unit vector. This way, we save the norm_square calculation, the addition and the division when computing the vector_to_rotating_axis variable. (Simply cleaner in my opinion) We throw an error is the vector is defined as the null vector which wasn't the case previously. How Has This Been Tested? All test pass. Throw error works Documentation The set rotation vector line in the documentation and examples were updated. Future changes The translation speed should be define as such in the parameter file Comments None Former-commit-id: f7f4cc8
blaisb
added a commit
that referenced
this pull request
Sep 30, 2024
Description of the problem The calculation of the translational velocity on rotating walls was calculated inadequately #896: The velocity of rotating boundary conditions (e.g. boundaries of the mesh) was calculated inadequately in the case where the normal vector of the wall was aligned with the rotation axis. The whole calculation procedure was slightly messed up and only worked for cylinders. Description of the solution This has been fixed and made general by using the vector from the contact point to the center of rotation. A new parameter has been added for this purpose. How Has This Been Tested? All test pass and give adequate results A new application test has been added to test this exact feature Documentation Documentation of the point on the axis has been updated Future changes I will continue to refactor the entire DEM particle-wall aspect to make it cleaner and more streamlined Co-authored-by: OGaboriault <[email protected]> Former-commit-id: 82a8a36
blaisb
pushed a commit
that referenced
this pull request
Sep 30, 2024
#Description of the problem This is a follow up to Dem particle wall sliding velocity #896 The rotational vector for the rotational boundary condition in the lethe-particles solver is now define with one line in the parameters file. We make sure that the rotational vector is a unit vector. This way, we save the norm_square calculation, the addition and the division when computing the vector_to_rotating_axis variable. (Simply cleaner in my opinion) We throw an error is the vector is defined as the null vector which wasn't the case previously. How Has This Been Tested? All test pass. Throw error works Documentation The set rotation vector line in the documentation and examples were updated. Future changes The translation speed should be define as such in the parameter file Comments None Former-commit-id: f7f4cc8
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of the problem
Description of the solution
How Has This Been Tested?
Documentation
Future changes