-
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
Add advection to the adaptive sparse contacts (ASC) #1113
Conversation
d7ce118
to
42157e9
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.
Few files already done, wil continue tomorrow
applications_tests/lethe-fluid-particles/adaptive_sparse_contacts_files/dem.pvdhandler
Outdated
Show resolved
Hide resolved
|
||
The dynamic disabling controls the disabling contact mechanism for performance enhancement. This feature dynamically searches for cells with low particle motion (granular temperature), disabling the computation of contacts for particles within these cells. | ||
The ASC controls the disabling contact mechanism for performance enhancement. This feature adaptively searches for cells with low particle motion (granular temperature), disabling the computation of contacts for particles within these cells. |
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 put your article on Arxiv and. you could reference it 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 will keep that in mind, and will do a micro-PR to add the reference :)
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.
Bunch of more files.
Thus far this is going well. this is a well done PR, very good quality work. Great job
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.
Last batch of comments. Good job @acdaigneault
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 addition and very nice and clear documentation on comments. I will checkpoint here and try to finish it Today. Otherwise, I will wrap it up tomorrow.
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.
Other than Victor's comment, that I agree with, in my opinion this will be ready for merge after all the reviews
|
||
subsection simulation control | ||
set method = bdf1 | ||
set number mesh adapt = 0 |
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.
Agreed
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 more comments as well. Good job, really! I will approve it after you have addressed the previous comments.
Clean up main algo (remove irrelevant checks)
Few test fixes
85f1610
to
b3538f2
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.
Amazing figure, amazing documentation, amazing feature. Good job!
Very last comment, but the rest is all fine.
P.S.: My bad for the example of lines in the comment, but the cascade thingy is 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.
You set the bar too high, you know?! Amazing, really!
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.
One comment, though: for the dark mode, if you do not set a bg, we won't be able to read the content of the label. Could you add a white bg to your figure?
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.
Ohhh good point, I will do it tonight or tomorrow :)
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.
Damn, thats too much flatteries for me hahaha ❤️
Description of the problem Periodic nodes were not properly handled with the last implementation: the periodic nodes did not have information on the mobility status on the coincident nodes. This was spotted when there has particles at one side of the periodic cells and the other periodic ones were empty. Those cells where not mobile, but this is one of the criteria for mobile. Disabling contact of particles was functional in CFD-DEM: cells with low granular temperature deactivated the contacts. However, it is possible for particles within a cell to have a low variance in velocity and a significant average velocity, such as when particles experience bulk motion. Description of the solution Periodic nodes: Mapping periodic nodes was achieved with the constraints since we can identify the periodic nodes from it (so called identity constraints). Some attempted as been done to apply the periodic constraints to the mobility_at_nodes, without any success. The main issue is that constraints force values at PB1 = values at PB2. In our case, we need to check of the prevailing value, so sometime it is PB2 = PB1, with was not compatible with straight constraints operations. "[...] it adds the DoFs of face_1 to the list of constrained DoFs in constraints and adds entries to constrain them to the corresponding values of the DoFs on face_2. " see Deal.II doc here Disabling contacts with advection: New mobility status has been added to the disabling contacts: advected and advected_active The new algorithm doesn't detect when particles are advected, this is a user-defined parameter. It could be useful for DEM when particles are moving in bulk by gravity, but this is more practical for CFD-DEM. The first idea was to implement advected particles auto-detection, but this results in more checks, also the average velocities threshold is not easly configurable. The way to "apply advection" is that the average velocity and acceleration of the cells are calculated or updated everytime the mobile cells are integrated. The average values are calculated in the same loop than the integration and it allows to always have the last calculated velocity values. For advected cells, all particles of a cell are integrated with an cell-averaged velocity and acceleration * time step. This velocity is also updated with the a*dt values. How Has This Been Tested? DEM tests for ASC was been updated New test in CFD-DEM called adaptative_sparse_contacts.prm Documentation Change the "disabling contacts" parameters to "adaptive sparse contacts" Doc is updated with new parameters and new names Comments This algorithm was called Disabling Contacts when it was first introduce in the code for DEM An example will be added in another PR Former-commit-id: 81fb7fa
Description of the problem Periodic nodes were not properly handled with the last implementation: the periodic nodes did not have information on the mobility status on the coincident nodes. This was spotted when there has particles at one side of the periodic cells and the other periodic ones were empty. Those cells where not mobile, but this is one of the criteria for mobile. Disabling contact of particles was functional in CFD-DEM: cells with low granular temperature deactivated the contacts. However, it is possible for particles within a cell to have a low variance in velocity and a significant average velocity, such as when particles experience bulk motion. Description of the solution Periodic nodes: Mapping periodic nodes was achieved with the constraints since we can identify the periodic nodes from it (so called identity constraints). Some attempted as been done to apply the periodic constraints to the mobility_at_nodes, without any success. The main issue is that constraints force values at PB1 = values at PB2. In our case, we need to check of the prevailing value, so sometime it is PB2 = PB1, with was not compatible with straight constraints operations. "[...] it adds the DoFs of face_1 to the list of constrained DoFs in constraints and adds entries to constrain them to the corresponding values of the DoFs on face_2. " see Deal.II doc here Disabling contacts with advection: New mobility status has been added to the disabling contacts: advected and advected_active The new algorithm doesn't detect when particles are advected, this is a user-defined parameter. It could be useful for DEM when particles are moving in bulk by gravity, but this is more practical for CFD-DEM. The first idea was to implement advected particles auto-detection, but this results in more checks, also the average velocities threshold is not easly configurable. The way to "apply advection" is that the average velocity and acceleration of the cells are calculated or updated everytime the mobile cells are integrated. The average values are calculated in the same loop than the integration and it allows to always have the last calculated velocity values. For advected cells, all particles of a cell are integrated with an cell-averaged velocity and acceleration * time step. This velocity is also updated with the a*dt values. How Has This Been Tested? DEM tests for ASC was been updated New test in CFD-DEM called adaptative_sparse_contacts.prm Documentation Change the "disabling contacts" parameters to "adaptive sparse contacts" Doc is updated with new parameters and new names Comments This algorithm was called Disabling Contacts when it was first introduce in the code for DEM An example will be added in another PR Former-commit-id: 81fb7fa
Description of the problem
Periodic nodes were not properly handled with the last implementation: the periodic nodes did not have information on the mobility status on the coincident nodes. This was spotted when there has particles at one side of the periodic cells and the other periodic ones were empty. Those cells where not mobile, but this is one of the criteria for mobile.
Disabling contact of particles was functional in CFD-DEM: cells with low granular temperature deactivated the contacts. However, it is possible for particles within a cell to have a low variance in velocity and a significant average velocity, such as when particles experience bulk motion.
Description of the solution
Periodic nodes:
mobility_at_nodes
, without any success. The main issue is that constraints force values at PB1 = values at PB2. In our case, we need to check of the prevailing value, so sometime it is PB2 = PB1, with was not compatible with straight constraints operations. "[...] it adds the DoFs of face_1 to the list of constrained DoFs in constraints and adds entries to constrain them to the corresponding values of the DoFs on face_2. " see Deal.II doc hereDisabling contacts with advection:
How Has This Been Tested?
Documentation
Comments