Introduce the DEM action manager #1206

acdaigneault · 2024-07-24T20:04:01Z

Description

Introducing singleton design pattern with the DEM action manager.
In DEM, we often check if an action has to be performed (insertion, load balancing, contact detection, etc), and those action result in resetting containers and/or do a particle contact search. The way it was handled was turning into a spaghetti code with the addition of new features.
Before, when an event was performed, a flag was set to true. And then a few places in the code was checking the flag to know if the current action has to be executed.
It was getting weird because some flags where nested into others if they triggered the same actions, but the flag has lost its meaning (ex: checkpoint in load balance flag, same for the solid object mapping).

In order to overcome this,, the action manager is introduced in the DEM and CFD-DEM to manage what event triggered what.
How does it works? Any event that needs some following actions are handled by the action manager. The event communicates to the action manager that it happened and the action manager will change the trigger flags to the actions to do. When this action is about to be performed, it checks with the action manager if it should be trigger.
Many events can triggered the same actions, but when the action is about to be performed, it only checks if it has to be performed, not what event have happened.

Many checks in the DEM and CFDDEM solvers were moved into the called function, even though the feature might not be implemented. The first thing in the function is a check with a return if so.

Testing [WIP]

The following tests have changed because the displacement vector was not reset after a contact search for every cases: restart-gas-solid-fluidized-bed.mpirun=2.output. dynamic_contact_search.mpirun=1.output.

load_balancing_solid_mobiliity_status=2.output: it seems also be related to the contact search that is shifted by one iteration. The difference between the old results with LB and without LB balancing are more different than the difference in this PR.
load_balancing_solid_mobiliity_status=2.output

Miscellaneous (will be removed when merged)

I found a few bugs-ish where checks were incoherents
There's also a lot of splitting of steps into functions, it was easier to handle, but I managed to make the DEM and the CFD-DEM almost the same that way. Then, I was able to add the action manager to the CFD-DEM also, and it would help to maybe add the DEM solver into the CFDDEM solver and only use the functions of the DEM.

Bugs found, but not fixed:

Display of new particles distribution after load balancing is wrong, it shows the previous distribution since particle_handler is not updated until "sort_particles_into_subdomains_and_cells".
All DEM iteration in the first CFD iteration use half step?
The tangential overlaps for particles-walls are not explicitly reset after a load balancing as done for particles.

Refactoring suggestions:

Would be nice to have a dem_force_manager as done for contacts instead of the function in DEM.
Use the DEM solver as an object of CFD-DEM solver or double inheritance.

I did most of the Doxygen doc of the CFD-DEM, but will continue in a other PR for the variables.
I still have a small detail I want to change, but it is ready for review.

Checklist (will be removed when merged)

See this page for more information about the pull request process.

Code related list:

All in-code documentation related to this PR is up to date (Doxygen format)
Lethe documentation is up to date
The branch is rebased onto master
Code is indented with indent-all and .prm files (examples and tests) with prm-indent
If parameters are modified, the tests and the documentation of examples are up to date
Changelog (CHANGELOG.md) is up to date if the refactor affects the user experience or the codebase

Pull request related list:

No other PR is open related to this refactoring
Labels are applied
There are at least 2 reviewers (or 1 if small feature) excluding the responsible for the merge
If this PR closes an issue or is related to a project, it is linked in the "Projects" or "Development" section
If any future works is planed, an issue is opened
The PR description is cleaned and ready for merge

applications_tests/lethe-fluid-particles/spouted_bed_load_balancing.mpirun=2.output

blaisb

First batch of comments, will continue. Very good owrk though

blaisb · 2024-07-26T20:45:17Z

include/dem/dem_action_manager.h

+
+  /**
+   * @brief Reset all triggers to false.
+   * TODO


TODO here being setting the documentation?

I want to remove the contact_search_trigger = mobility_status_reset_trigger; because it's sketchy and not clear haha

include/dem/dem_action_manager.h

blaisb · 2024-07-26T20:51:02Z

include/dem/dem_action_manager.h

+   *                   calculation.
+   */
+  inline void
+  last_dem_of_cfd_iteration_step()


maybe last_dem_of_cfd_dem_coupling_step()?
I know it's a bit long, but it's clearer I find

I renamed to last_dem_of_cfddem_iteration_step()
last_dem_of_cfd_dem_coupling_step() sounds more like it is the very last one of the simulation in the CFD-DEM sovler

blaisb

I went through all the files once. Only minor comments. I will take a look again once theses changes (small ones really) are addressed :)

include/dem/periodic_boundaries_manipulator.h

include/fem-dem/cfd_dem_coupling.h

source/dem/CMakeLists.txt

source/dem/dem_contact_manager.cc

source/dem/explicit_euler_integrator.cc

source/dem/gear3_integrator.cc

source/fem-dem/cfd_dem_coupling.cc

OGaboriault

not finish to review

include/dem/dem.h

include/dem/adaptive_sparse_contacts.h

OGaboriault · 2024-07-29T06:01:01Z

include/dem/dem.h

+  DEM::DEMProperties<dim> properties_class;
+
+  /**
+   * @brief The acceleration acting of the particles.


Suggested change

* @brief The acceleration acting of the particles.

* @brief The gravitational acceleration acting on the particles.

Theoretically, it can be other forces (let's say from a electromagnetic field). Like, we use it for gravity and the letter it g, but stilll!

voferreira

Super cool! I commented some very minor things but all clear to me.

include/dem/dem_action_manager.h

voferreira · 2024-07-29T12:45:37Z

include/dem/dem_action_manager.h

+  }
+
+  /**
+   * @brief Check if the periodic boundaries are enabled to perform some actions.


Periodic boundaries checker? Minor thing but "to perform some actions" is super vague :P

It is a bit vague on purpose, because those actions are not in the class, meaning that they will probably won't be updated here even tho I write the current ones. But I did added "... that are not handled by triggers of the action manager."

blaisb · 2024-07-30T13:32:30Z

@acdaigneault do you know what is the origin of the two failing tests?

acdaigneault · 2024-07-30T13:44:35Z

@acdaigneault do you know what is the origin of the two failing tests?

Yeah I fixed it last night, seems I did not push it correctly

Cleanup in initialization of parameter in DEM Rearrange setting order so it matches between dem and cfd-dem

Remove load-balancing step to cfd-dem Remove "step" flags, raw version

Here DEM and CFD-DEM works well

Refactor solid objects triggers

remove solid object flag Remove periodic flag Remove has_sparse_contacts flags Put feature abstration in dem manager Add restart abstraction level in dem

Reset contact search counter Remove PBC sort_in_cells at last time step

Fix tests

Add doxygen doc to DEM for functions

I think the solid object changed because the mapping of solid object and cells was not done after a load balancing before

Description Introducing singleton design pattern with the DEM action manager. In DEM, we often check if an action has to be performed (insertion, load balancing, contact detection, etc), and those action result in resetting containers and/or do a particle contact search. The way it was handled was turning into a spaghetti code with the addition of new features. Before, when an event was performed, a flag was set to true. And then a few places in the code was checking the flag to know if the current action has to be executed. It was getting weird because some flags where nested into others if they triggered the same actions, but the flag has lost its meaning (ex: checkpoint in load balance flag, same for the solid object mapping). In order to overcome this,, the action manager is introduced in the DEM and CFD-DEM to manage what event triggered what. How does it works? Any event that needs some following actions are handled by the action manager. The event communicates to the action manager that it happened and the action manager will change the trigger flags to the actions to do. When this action is about to be performed, it checks with the action manager if it should be trigger. Many events can triggered the same actions, but when the action is about to be performed, it only checks if it has to be performed, not what event have happened. Many checks in the DEM and CFDDEM solvers were moved into the called function, even though the feature might not be implemented. The first thing in the function is a check with a return if so. Co-authored-by: Bruno Blais <blais.bruno@gmail.com> Former-commit-id: 63f64ab

acdaigneault self-assigned this Jul 24, 2024

acdaigneault added Enhancement New feature or request WIP When a PR is open but not ready for review Refactoring This PR is only refactoring or clean up labels Jul 24, 2024

acdaigneault force-pushed the dem_action-manager branch from ed2038f to d056c3d Compare July 24, 2024 21:15

blaisb reviewed Jul 25, 2024

View reviewed changes

applications_tests/lethe-fluid-particles/spouted_bed_load_balancing.mpirun=2.output Outdated Show resolved Hide resolved

acdaigneault force-pushed the dem_action-manager branch 2 times, most recently from 279c50a to f8f12c0 Compare July 26, 2024 16:04

acdaigneault added Ready for review and removed WIP When a PR is open but not ready for review labels Jul 26, 2024

acdaigneault requested review from blaisb and OGaboriault July 26, 2024 19:01

blaisb requested changes Jul 26, 2024

View reviewed changes

acdaigneault requested a review from voferreira July 27, 2024 16:30

acdaigneault force-pushed the dem_action-manager branch from 0f760b0 to ae0cb19 Compare July 27, 2024 18:32

blaisb requested changes Jul 28, 2024

View reviewed changes

OGaboriault reviewed Jul 29, 2024

View reviewed changes

voferreira approved these changes Jul 29, 2024

View reviewed changes

acdaigneault force-pushed the dem_action-manager branch from f72b1d7 to 1589b7f Compare July 29, 2024 22:30

blaisb approved these changes Jul 30, 2024

View reviewed changes

acdaigneault added 8 commits July 30, 2024 09:54

Load balance branch commit

57cc0fa

Comparable steps in dem and cfd-dem

1d2416f

Cleanup in initialization of parameter in DEM Rearrange setting order so it matches between dem and cfd-dem

Remove contact detection step to cfd-dem

6de31a2

Remove load-balancing step to cfd-dem Remove "step" flags, raw version

Fix contact detection reset and tests

b7ce570

Here DEM and CFD-DEM works well

Remove if for solid objects

9547deb

Refactor solid objects triggers

Put flags in class

6a844df

remove solid object flag Remove periodic flag Remove has_sparse_contacts flags Put feature abstration in dem manager Add restart abstraction level in dem

fix unit tests

31eef10

Force contact search at last time step

abaace7

Reset contact search counter Remove PBC sort_in_cells at last time step

acdaigneault added 16 commits July 30, 2024 09:54

Doxygen doc

f523bd9

Fix tests

Add grid motion to action manager pattern

23f3511

[wip] Clean up

9ee11fa

Raw application-tests output

63ea8d0

Reorganize functions in dem

c01398f

Add doxygen doc to DEM for variables

9d45dde

Add doxygen doc to DEM for functions

Only resize container after a LB

ba9b819

Fix test according to setup order

371688b

Change tests

c2b3eda

I think the solid object changed because the mapping of solid object and cells was not done after a load balancing before

update test to master

70df1bb

Do doxygen doc for cfd-dem for functions

576d1b4

Apply first Bruno's comments

d86a41c

Bad doc indent

3a9bcdb

Apply quick suggestions

2c94e90

Apply other suggestions

e52277e

Small fix

22ef4d7

acdaigneault force-pushed the dem_action-manager branch from 699308a to 22ef4d7 Compare July 30, 2024 13:55

blaisb added Reviewed and ready to merge and removed Ready for review labels Jul 30, 2024

Update benchmark and timings of benchmarks

4b51a37

blaisb merged commit 63f64ab into master Jul 30, 2024
8 checks passed

blaisb deleted the dem_action-manager branch July 30, 2024 18:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce the DEM action manager #1206

Introduce the DEM action manager #1206

acdaigneault commented Jul 24, 2024 •

edited

Loading

blaisb left a comment

blaisb Jul 26, 2024

acdaigneault Jul 27, 2024

blaisb Jul 26, 2024

acdaigneault Jul 27, 2024

blaisb left a comment

OGaboriault left a comment

OGaboriault Jul 29, 2024

acdaigneault Jul 29, 2024

blaisb Jul 30, 2024

voferreira left a comment

voferreira Jul 29, 2024

acdaigneault Jul 29, 2024

blaisb commented Jul 30, 2024

acdaigneault commented Jul 30, 2024

	* @brief The acceleration acting of the particles.
	* @brief The gravitational acceleration acting on the particles.

Introduce the DEM action manager #1206

Introduce the DEM action manager #1206

Conversation

acdaigneault commented Jul 24, 2024 • edited Loading

Description

Testing [WIP]

Miscellaneous (will be removed when merged)

Checklist (will be removed when merged)

blaisb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blaisb left a comment

Choose a reason for hiding this comment

OGaboriault left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voferreira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blaisb commented Jul 30, 2024

acdaigneault commented Jul 30, 2024

acdaigneault commented Jul 24, 2024 •

edited

Loading