Fix bugs: changing time step after restart and tracer tanh model #1343

oguevremont · 2024-11-07T14:59:04Z

Description

There were two issues that appeared when using the tracer physics with an immersed solid:

Evaluation of the diffusivity model immersed solid tanh was too costly
Changing the time step after restarting was impossible

Solution

The diffusivity model now uses the optimizations from the Shape class, that allow to evaluate the distance using a cell guess; this makes RBF and composite distance evaluations magnitudes faster.
Time step change after a restart is now allowed when adapt==false. This behavior is not allowed when adapt==true, as then we desire a continuity in time step evolution. When using fixed time step though, users who change the value after a restart apparently aim for this behavior.

Testing

All tests still pass.

Documentation

No change needed.

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:

All in-code documentation related to this PR is up to date (Doxygen format)
Copyright headers are present and up to date
Lethe documentation is up to date
Fix has unit test(s) (preferred) or application test(s), and restart files are in the generator folder
The branch is rebased onto master
Changelog (CHANGELOG.md) is up to date
Code is indented with indent-all and .prm files (examples and tests) with prm-indent

Pull request related list:

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 the fix is temporary, an issue is opened
The PR description is cleaned and ready for merge

lpsaavedra

I have a few comments and doubts regarding these new features

CHANGELOG.md

lpsaavedra · 2024-11-08T15:48:55Z

include/solvers/tracer_scratch_data.h

@@ -159,7 +159,7 @@ class TracerScratchData : public PhysicsScratchDataBase
         const VectorType                                     &current_solution,
         const std::vector<VectorType> &previous_solutions,
         const Function<dim>           *source_function,
-         const Function<dim>           *levelset_function)
+         Shape<dim>                    *immersed_solid_shape)


Shouldn't it be const?

Because we use value_with_cell_guess, no. value_with_cell_guess is not const, which allows us to add entries to a map in a caching mechanism. Without using this mechanism the code runs much too slowly.

I agree that it would be better to use a const function, but I do not see an efficient way to populate the map otherwise; we need to populate the map with the value of the distance for all quadrature points in the mesh. Maybe we could do it once, with a loop over all active cells before reinitializing the scratch? Then we could use value instead, which is const and can use an already populated map

lpsaavedra · 2024-11-08T15:50:27Z

include/solvers/tracer_scratch_data.h

@@ -317,7 +320,7 @@ class TracerScratchData : public PhysicsScratchDataBase
    const unsigned int                                   &neigh_face_no,
    const unsigned int                                   &neigh_sub_face_no,
    const VectorType                                     &current_solution,
-    const Function<dim>                                  *levelset_function)
+    const Function<dim>                                  *immersed_solid_shape)


Isn't this a shape? According to the previous changes?

lpsaavedra · 2024-11-08T15:51:01Z

include/solvers/tracer_scratch_data.h

@@ -383,7 +387,7 @@ class TracerScratchData : public PhysicsScratchDataBase
    const unsigned int                                   &face_no,
    const unsigned int                                   &boundary_index,
    const VectorType                                     &current_solution,
-    const Function<dim>                                  *levelset_function)
+    Shape<dim>                                           *immersed_solid_shape)


Shouldn't it be const?

Suggested change

Shape<dim> *immersed_solid_shape)

const Shape<dim> *immersed_solid_shape)

lpsaavedra · 2024-11-08T15:58:39Z

source/core/simulation_control.cc

+  // Fix the time step to the new provided value.
+  // We understand that users providing this value when adaptive time stepping
+  // is not enabled means a time step change is the desired effect.
+  if (!adapt)
+    set_current_time_step(initial_time_step);


I don't fully understand why is this useful? Like in what kind of cases...Isn't it "risky" to change the time step in the middle of the simulation when restarting it?

Yes it is risky business, but the way I use it is that I solve NS with small time steps, until the solution is steady, then I solve only tracer with large time steps after a restart (before and after the restart the time step is fixed). I should write this somewhere so it is clearer, thanks for pointing it out

Co-authored-by: Laura Prieto Saavedra <[email protected]>

lpsaavedra

I just have one additional minor comment...

CHANGELOG.md

Co-authored-by: Laura Prieto Saavedra <[email protected]>

blaisb

Much better this way I like it

oguevremont added Bug Something isn't working WIP When a PR is open but not ready for review labels Nov 7, 2024

oguevremont requested a review from lpsaavedra November 7, 2024 16:43

oguevremont added Ready for review and removed WIP When a PR is open but not ready for review labels Nov 8, 2024

oguevremont marked this pull request as ready for review November 8, 2024 15:57

lpsaavedra requested changes Nov 8, 2024

View reviewed changes

oguevremont mentioned this pull request Nov 14, 2024

New example: tracer in static mixer #1362

Merged

11 tasks

oguevremont and others added 9 commits November 15, 2024 02:39

Changed evaluation of the levelset for tracer diffusivity model

0015401

Quick fix for time step

ee87512

Remove value without cell guess call

4cc57b0

Added doxygen param [in]

37f2a6b

Update changelog

7fbfad8

Change function to shape

6209f52

Add [in]

b65999a

Address review comments

2bcbac3

Apply suggestions from code review

b5735e6

Co-authored-by: Laura Prieto Saavedra <[email protected]>

oguevremont force-pushed the fix_tracer_tanh branch from f4dc07a to b5735e6 Compare November 15, 2024 07:51

oguevremont added 6 commits November 15, 2024 03:12

Remove duplicate read functio

7bd2c2e

Split reinitialization of the sdf

4a92df1

Split reinitialization of the sdf for faces also

5fb14c7

Cleaned shape usage

2fb060d

Remove useless call

847d4eb

Update changelog

0cb49f8

lpsaavedra approved these changes Nov 18, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

c2c0dec

Co-authored-by: Laura Prieto Saavedra <[email protected]>

oguevremont requested a review from blaisb November 18, 2024 15:16

blaisb approved these changes Nov 18, 2024

View reviewed changes

blaisb merged commit 682ab4b into master Nov 18, 2024
11 checks passed

blaisb deleted the fix_tracer_tanh branch November 18, 2024 16:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bugs: changing time step after restart and tracer tanh model #1343

Fix bugs: changing time step after restart and tracer tanh model #1343

oguevremont commented Nov 7, 2024 •

edited

Loading

lpsaavedra left a comment

lpsaavedra Nov 8, 2024

oguevremont Nov 12, 2024

lpsaavedra Nov 8, 2024

lpsaavedra Nov 8, 2024

lpsaavedra Nov 8, 2024

oguevremont Nov 12, 2024

lpsaavedra left a comment

blaisb left a comment

	Shape<dim> *immersed_solid_shape)
	const Shape<dim> *immersed_solid_shape)

Fix bugs: changing time step after restart and tracer tanh model #1343

Fix bugs: changing time step after restart and tracer tanh model #1343

Conversation

oguevremont commented Nov 7, 2024 • edited Loading

Description

Solution

Testing

Documentation

Miscellaneous (will be removed when merged)

Checklist (will be removed when merged)

lpsaavedra left a comment

Choose a reason for hiding this comment

lpsaavedra Nov 8, 2024

Choose a reason for hiding this comment

oguevremont Nov 12, 2024

Choose a reason for hiding this comment

lpsaavedra Nov 8, 2024

Choose a reason for hiding this comment

lpsaavedra Nov 8, 2024

Choose a reason for hiding this comment

lpsaavedra Nov 8, 2024

Choose a reason for hiding this comment

oguevremont Nov 12, 2024

Choose a reason for hiding this comment

lpsaavedra left a comment

Choose a reason for hiding this comment

blaisb left a comment

Choose a reason for hiding this comment

oguevremont commented Nov 7, 2024 •

edited

Loading