Skip to content
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 channel-transport transport OpenFOAM #551

Open
wants to merge 18 commits into
base: develop
Choose a base branch
from

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.

Note that this solver conserves the overall T, leading to an accumulation of the concentration close to the outlet.

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

Result:

coarse-blob.mp4

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.

@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.

@@ -41,6 +41,8 @@ Transport participant:

* Nutils. For more information, have a look at the [Nutils adapter documentation](https://precice.org/adapter-nutils.html). This Nutils solver requires at least Nutils v7.0.

* OpenFOAM. This case uses a modified version of scalarTransportFoam, which recomputes `phi` after the preCICE adapter reads the velocity field. Build it with `wmake`. Read more details in the [OpenFOAM adapter](https://precice.org/adapter-openfoam-overview.html). Note that T accumulates in front of the outlet as this case conserves overall T.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* OpenFOAM. This case uses a modified version of scalarTransportFoam, which recomputes `phi` after the preCICE adapter reads the velocity field. Build it with `wmake`. Read more details in the [OpenFOAM adapter](https://precice.org/adapter-openfoam-overview.html). Note that T accumulates in front of the outlet as this case conserves overall T.
* OpenFOAM. This case uses a modified version of scalarTransportFoam, which recomputes `phi` after the preCICE adapter reads the velocity field. Build it with `wmake`. Read more details in the [OpenFOAM adapter](https://precice.org/adapter-openfoam-overview.html). Note that `T` accumulates in front of the outlet as this case conserves overall `T`.

For consistency.

However: is this really intended, or a modelling issue?

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 a modelling issue that I couldn't find a solution for. I also ran out of people to ask for help

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 better understand this, and either add details, or fix it. I think right now it just looks wrong.

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).

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)

divSchemes
{
default none;
div(phi,T) Gauss linearUpwind grad(T);
Copy link
Member

Choose a reason for hiding this comment

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

Does tuning this maybe help with the strange values at the outlet?

Copy link
Member Author

Choose a reason for hiding this comment

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

Feel free to try. I wasn't able to change the behaviour.

Copy link
Member

Choose a reason for hiding this comment

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

We can eventually try this together, but I don't have the time to debug this at the moment. That's why I am giving a hint on what I think could help first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants