Skip to content

Conversation

@fsimonis
Copy link
Member

@fsimonis fsimonis commented Jul 11, 2024

This PR adds an OpenFOAM variant of the transport participant of the channel-transport tutorial.

The case uses a modified version of the scalarTransportFoam application. Main difference is that we assume U to change every timestep. Therefore we need to recompute $\phi$ every timestep.

@MakisH Can you give this PR a thorough review to make it as easy to use as possible?

Result:

Screencast_20251210_145936.webm

Checklist:

  • I added a summary of any user-facing changes (compared to the last release) in the changelog-entries/<PRnumber>.md.
  • I will remember to squash-and-merge, providing a useful summary of the changes of this PR.

edit: updated to the working state

@fsimonis fsimonis requested a review from MakisH July 11, 2024 11:20
@fsimonis fsimonis self-assigned this Jul 11, 2024
Copy link
Member

@MakisH MakisH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides the fact that solver and config are in the same directory, it looks good overall. I have not yet tried to run it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I start feeling very uneasy that we duplicate this file everywhere.
Opened an issue: #559

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no uniform style in the tutorials repo. I don't see an easy solution for this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a .clang-format file in the root directory: https://github.com/precice/tutorials/blob/develop/.clang-format

Let's discuss in #559.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have these in the central gitignore: https://github.com/precice/tutorials/blob/develop/.gitignore

The 0/ is already covered, the rest could go there as-is (strange we don't have them already).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the other way around.

0/ is tracked and 0.*/ ignored by default. In my case I generate 0/ from 0.orig/, which is the exact opposite of what the gitignore defines.

tutorials/.gitignore

Lines 41 to 43 in 3f253cc

0.*/
[1-9]*/
!0/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly, we don't have a .gitignore in https://github.com/precice/tutorials/tree/develop/partitioned-pipe-two-phase, which should have the same issue.

But again, the Make/-related rules can go into the central .gitignore, or at least moved to the solver directory.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain the exact history of this file?
What did you get from where, what did you add yourself?

Copy link
Member Author

@fsimonis fsimonis Sep 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where would you expect to find this information? In the file itself or in a README in the directory of the solver?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just here first, to decide on what to do with this header.
But eventually in the file itself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment here would be nice.
I assume you are initializing a blob of T at (1,1).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines +1 to +7
/*--------------------------------*- C++ -*----------------------------------*\
| ========= | |
| \\ / F ield | OpenFOAM: The Open Source CFD Toolbox |
| \\ / O peration | Version: v2312 |
| \\ / A nd | Website: www.openfoam.com |
| \\/ M anipulation | |
\*---------------------------------------------------------------------------*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These headers are actually misleading, since we anyway have changed these configuration files a lot compared to what you might have started from.

(referring to this file and the fvSolution)

Copy link
Member Author

@fsimonis fsimonis Sep 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you suggest me to handle this? (also in the solver source)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am suggesting to remove the copyright header from the fvSchemes and fvSolution, because it is wrong.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the actual solver sources?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There we need it, since it is actually a small extension of OpenFOAM code.
But this is discussed in #551 (comment)

Copy link
Member Author

@fsimonis fsimonis Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replaced the header with a link and description. Does this clarify the matter?

@MakisH
Copy link
Member

MakisH commented Jul 17, 2025

@fsimonis this seems to be close to merging. Does this need more work from your side, or just a new review iteration?

edit: Actually, no. There is still a blocking modeling issue.

@MakisH
Copy link
Member

MakisH commented Nov 26, 2025

Just finished a co-working session with @fsimonis. I pushed some new commits, which make the custom solver work with dynamic meshes and adjusts the case to apply dynamic mesh refinement with dynamicRefineFvMesh, based on the magnitude of the gradient of the transported value.

The settings are in constant/dynamicMeshDict and are currently rather arbitrary. Next step would be to refine these. We worked with an uncoupled scalar transport case based on the pitzDaily tutorial. Here is the draft (also with arbitrary settings):

adaptivePitzDaily.tar.gz

Run with: rm -rf 0.* && blockMesh && dynamicScalarTransportFoam.

The next step would be to further understand and adjust the mesh refinement settings.

The coupled case currently fails, but this is expected since the adapter does not yet support remeshing (precice/openfoam-adapter#382). I can start an implementation of that.

The accumulation of T at the outlet remains. Making the channel longer and refining the mesh showed that the accumulation is really on the last 2-3 layers of the mesh. Changing the system/fvSchemes strongly affected the behavior (to the worse):

divSchemes
{
    default         none;
-    div(phi,T)      Gauss linearUpwind grad(T);
+    div(phi,T)      Gauss linear grad(T);
}

However, the numerics and the boundary conditions are the same as in the pitzDaily case, so this might be unrelated. I would look next if the changes from the standard scalarTransportFoam have any effect (but I would expect not).

Some URLs for reference:

@fsimonis fsimonis mentioned this pull request Dec 10, 2025
2 tasks
@fsimonis
Copy link
Member Author

I got the non-AMR version running. I moved the AMR specific changes to #689 and will revert this PR to the non-AMR version.
Let's then first merge it and continue from there.

@fsimonis fsimonis force-pushed the add-channel-transport-openfoam branch from b1f4d6a to 44e2384 Compare December 10, 2025 14:25
@fsimonis
Copy link
Member Author

@MakisH I think I implemented all suggestions. Could you review again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants