Add partitioned-heat OpenFOAM participant with solver #223

davidscn · 2021-06-25T16:32:33Z

Since #214 is a far away from its purpose and includes various changes I don't understand let me add a clean PR. The tutorial requires groovyBC in order to handle the time dependent inhomogenous Dirichlet boundary condition.
Open to discuss: this PR comprises the solver as well.

MakisH · 2021-06-25T18:28:35Z

Are the solver files part of OpenFOAM? If yes, why include them here? (guessing from the copyright notices)

davidscn · 2021-06-25T19:04:12Z

No they are not, just started from OpenFOAM own files,

uekerman · 2021-06-28T14:18:32Z

@davidscn Could you also please quickly comment here which results you get? (I did not run the case yet). Do you get the correct results when e.g. coupling to FEniCS?

Edit: Just saw that you already added the results in precice/openfoam-adapter#93

uekerman · 2021-06-28T14:19:55Z

Open to discuss: this PR comprises the solver as well.

We do the same for FEniCS and Nutils. If they are not part of OpenFOAM, the right way to got is indeed to add them here in my opinion.

MakisH · 2021-06-28T14:24:17Z

We do the same for FEniCS and Nutils. If they are not part of OpenFOAM, the right way to got is indeed to add them here in my opinion.

Definitely, but currently the copyright notices look as if these solvers are part of OpenFOAM v2012, that's why I got confused.

uekerman · 2021-06-28T14:25:31Z

Definitely, but currently the copyright notices look as if these solvers are part of OpenFOAM v2012, that's why I got confused.

I see. How should / could we change the copyright notice? Just remove?

MakisH · 2021-06-28T15:21:14Z

Definitely, but currently the copyright notices look as if these solvers are part of OpenFOAM v2012, that's why I got confused.

I see. How should / could we change the copyright notice? Just remove?

@davidscn knows best here. If the code is mostly from OpenFOAM with some small changes, then keep the headers as they are and write some "extended by" (David, preCICE, whatever policy we want to have). If the code is mostly new, then I don't see any need for detailed copyright notices, maybe only an attribution such as "based on previous work on X by Y".

davidscn · 2021-06-29T06:13:25Z

I wrote some custom text in order to highlight from where we started..

davidscn · 2021-07-13T14:57:45Z

This PR is now ready to go. As documented in the issue, it works as expected. I only need to update the documentation if we want to merge it. Any volunteer how wants to try out the actual tutorial (groovyBC required)?

uekerman · 2021-07-14T10:34:18Z

I only need to update the documentation if we want to merge it.

To which documentation are you referring?

Let's wait for the changelog PR from @IshaanDesai such that we can also document this change.

davidscn · 2021-07-14T10:38:59Z

To which documentation are you referring?

https://precice.org/tutorials-partitioned-heat-conduction.html

uekerman · 2021-07-14T10:41:27Z

I need some documentation to build the OpenFOAM solvers 😁
Could you please update the documentation already?
Also run scripts are missing.

A new inconsistency issue is now the folder structure 😕

fenics
nutils
openfoam-dirichlet
openfoam-neumann

davidscn · 2021-07-14T12:51:04Z

A new inconsistency issue is now the folder structure confused

fenics
nutils
openfoam-dirichlet
openfoam-neumann

Hm, not sure which part is inconsistent. Merging the OpenFOAM cases is, however, not possible as long as we explicitly want to run the OpenFOAM-OpenFOAM coupling.

EDIT: We also have the problem where to put the solver files. Duplicating them into both OF cases is certainly a bad idea.

MakisH · 2021-07-14T18:01:39Z

I think that fenics and nutils are the special case here because they needed to store the solver together with the case.

We have a kind-of-similar situation in the elastic-tube-1d. Previously, the Python solvers were sharing some files, which we merged into the solvers to not have this issue (e.g. hard-code parameters, instead of using configuration files, move the tools to the root directory). If we have the duplication there, we could also have it here, if it helps the structure.

An alternative we already use in the repository to avoid duplications is symbolic links.

Another alternative would be to have the solvers in a tools/ directory in the case and then the run.sh script of each case would e.g. build and copy the solver binary before executing it. There are many alternatives in this direction, but this is one option.

uekerman · 2021-07-15T08:48:07Z

Yes, we could duplicate fenics and nutils into fenics-dirichlet, ...

... @BenjaminRodenberg might not like this idea? (I think we had this discussion already)

Symbolic links or tools/ feel a bit over-engineered to me here.

The other alternative is to keep it as it is and just be careful to properly document the "how to run" in the README.

MakisH · 2021-07-15T09:06:39Z

Yes, we could duplicate fenics and nutils into fenics-dirichlet, ...

... @BenjaminRodenberg might not like this idea? (I think we had this discussion already)

Yes, we had such a discussion already and I understand that this duplication avoidance was the original motivation.

My main counterargument was (and remains here) that special cases can lead again to a difficult to maintain situation and can make automation harder. But I completely understand that this is not a trivial case that fits nicely into our current structure.

Symbolic links or tools/ feel a bit over-engineered to me here.

For me too. However, I think the structure breaks because normally we assume that the solver already exists and the tutorial is only a set of config files. So, if we want to stick to the structure, we should try to separate the solver from the configs. One way is to have the solvers in a directory of a standardized name inside each tutorial case.

The other alternative is to keep it as it is and just be careful to properly document the "how to run" in the README.

I am afraid this will set a precedent for more special cases in the future, so I would prefer to adjust the system now.

BenjaminRodenberg · 2021-07-15T09:18:57Z

I quickly skimmed over this PR. I would suggest to merge the structure as it is fenics, nutils, openfoam (if possible) or fenics, nutils, openfoam-dirichlet, openfoam-neumann (if necessary). The discussion about whether we should restructure the nutils and fenics case does not really belong into this PR and should be moved to an issue in my opinion. If we do the restructuring in this PR, if will become gigantic. Therefore, let's try to move it into an issue / another PR as soon as possible.

Yes, we could duplicate fenics and nutils into fenics-dirichlet, ...
... @BenjaminRodenberg might not like this idea? (I think we had this discussion already)

Yes, we had such a discussion already and I understand that this duplication avoidance was the original motivation.

My main counterargument was (and remains here) that special cases can lead again to a difficult to maintain situation and can make automation harder. But I completely understand that this is not a trivial case that fits nicely into our current structure.

And yes. I don't like this idea 😅 We could stick to our structure by introducing some links (e.g. heat-dirichlet.py and heat-neumann.py are just links pointing to heat.py). But I think this is now an idea that @MakisH does not like (and probably @uekerman also?).

BenjaminRodenberg · 2021-07-15T09:24:57Z

And yes. I don't like this idea 😅

We could still draft the "duplication" idea. I can imagine that it also has some possible benefits. Right now heat.py has no duplication, but it is sometimes also a bit hard to read and convoluted due to the many if-statements. We can also create modules, where we put code that is common to both solvers and which would require a lot of duplication (e.g. https://github.com/precice/tutorials/blob/master/partitioned-heat-conduction/fenics/problem_setup.py).

davidscn · 2021-07-19T08:37:18Z

Ok, then I would continue with the following structure:

.
├── clean-tutorial.sh -> ../tools/clean-tutorial-base.sh
├── fenics
├── images
├── nutils
├── openfoam-dirichlet
├── openfoam-neumann
├── openfoam-solver
├── precice-config.xml
└── README.md

uekerman · 2021-07-19T10:23:12Z

Therefore, let's try to move it [the discussion] into an issue / another PR as soon as possible.

Please don't forget

BenjaminRodenberg · 2021-07-19T11:18:14Z

Therefore, let's try to move it [the discussion] into an issue / another PR as soon as possible.

Please don't forget

Done #228

davidscn · 2021-07-19T16:09:02Z

Ok, ready for review. I guess every potential reviewer is now already notified.

MakisH

A quick first review. Please trigger me again afterwards to try it.

partitioned-heat-conduction/openfoam-solver/Make/files

MakisH · 2021-07-19T16:18:06Z

partitioned-heat-conduction/openfoam-neumann/system/preciceDict

+
+CHT
+{
+   k   [ 1  1 -3 -1 0 0 0 ] -1;


Why the -1 here?

Compatibility. fenics and nutils have different sign conventions here.

Are we then restricted to which solver combinations we can run? Can the user still select between openfoam-openfoam, openfoam-nutils, fenics-openfoam? I understand that now only the openfoam-<other> and <other>-openfoam are valid cases now. Maybe we could find a more fundamental solution to this.

If I am looking at the right places, this is not documented.

Ah GitHub duplicated your comment, see the answer below.

Ah GitHub duplicated your comment, see the answer below.

I am not sure if we found a GitHub bug or if I commented in the wrong places, but something confusing is going on here.

I think you did not comment here but rather sent it with a new review. Too much confusion for GitHub. Anyway, let's consider the discussion.

Compatibility. fenics and nutils have different sign conventions here.

Sounds like sth we need to resolve. All combinations should be possible without changing any of the participants.

As documented below, all combinations work with the current setup.

partitioned-heat-conduction/openfoam-neumann/setInitialField.sh

partitioned-heat-conduction/openfoam-neumann/constant/transportProperties

partitioned-heat-conduction/openfoam-neumann/0/T

partitioned-heat-conduction/openfoam-dirichlet/system/preciceDict

partitioned-heat-conduction/openfoam-dirichlet/system/controlDict

partitioned-heat-conduction/openfoam-dirichlet/0/T

partitioned-heat-conduction/README.md

davidscn · 2021-07-20T07:20:20Z

A quick first review. Please trigger me again afterwards to try it.

Done and ready to try.

partitioned-heat-conduction/openfoam-dirichlet/system/controlDict

partitioned-heat-conduction/openfoam-neumann/system/controlDict

MakisH · 2021-07-20T07:41:25Z

partitioned-heat-conduction/openfoam-neumann/system/preciceDict

+
+CHT
+{
+   k   [ 1  1 -3 -1 0 0 0 ] -1;


Are we then restricted to which solver combinations we can run? Can the user still select between openfoam-openfoam, openfoam-nutils, fenics-openfoam? I understand that now only the openfoam-<other> and <other>-openfoam are valid cases now. Maybe we could find a more fundamental solution to this.

If I am looking at the right places, this is not documented.

davidscn · 2021-07-20T07:47:47Z

Are we then restricted to which solver combinations we can run? Can the user still select between openfoam-openfoam, openfoam-nutils, fenics-openfoam? I understand that now only the openfoam- and -openfoam are valid cases now. Maybe we could find a more fundamental solution to this.

If I am looking at the right places, this is not documented.

All combinations are valid as the sign cancels out for openfoam-openfoam

and adjust documentation as well as run scripts accordingly

Co-authored-by: Gerasimos Chourdakis <chourdak@in.tum.de>

MakisH

I also ran it now and it works as expected. If you want, consider these two additional points and feel free to merge.

partitioned-heat-conduction/README.md

partitioned-heat-conduction/openfoam-neumann/clean.sh

davidscn · 2021-07-29T11:01:40Z

Done, thanks @MakisH

davidscn linked an issue Jun 25, 2021 that may be closed by this pull request

Missing partitioned heat conduction tutorial with OpenFOAM #204

Open

davidscn self-assigned this Jul 13, 2021

BenjaminRodenberg mentioned this pull request Jul 19, 2021

Partitioned heat conduction structure is non-standard and inconsistent #228

Closed

davidscn force-pushed the partitioned-heat-OF branch from 2369874 to 8ecbbe2 Compare July 19, 2021 16:07

MakisH reviewed Jul 19, 2021

View reviewed changes

MakisH reviewed Jul 20, 2021

View reviewed changes

davidscn and others added 20 commits July 28, 2021 17:12

Add the OpenFOAM Neumann configuration

183029b

Fix shell description for initial field

b0a0777

Add modified solver version including non-zero RHS

bd8fc49

Format precice config according to our standard

07163bf

Rename 'Flux' to 'Heat-Flux' in precice-config.xml

ffc5ad5

Remove license header and write some custom text

2a7ba0d

Add non-saved modifications missing in the previous commit

8da8602

Add an additional Dirichlet participant

c477cc4

Fix some typos and missing brackets

9442b75

Add value slot in groovyBC for paraFoam reader

d4561cd

Add run and clean scripts and README description

545460c

Move openfoam solver in dedicated directory on the top level

05a00d9

Update documentation for updated structure

db188f2

Store generated executable in FOAM_USER_APPBIN

d9eb122

and adjust documentation as well as run scripts accordingly

Add a comment on the heat transfer coefficient

b30fa85

Fix duplicated 'in the' in controlDict

479d08e

Plug everything in one script to enable easy mesh modifications

c01e7a1

Adjust the documentation for updated workflows

963433d

Apply suggestions from Makis review

6537a4e

Co-authored-by: Gerasimos Chourdakis <chourdak@in.tum.de>

Remove OpenFOAM header from Temperature file

b85f577

davidscn force-pushed the partitioned-heat-OF branch from 8a0223c to b85f577 Compare July 28, 2021 15:13

Add a changelog entry

bccd675

MakisH approved these changes Jul 28, 2021

View reviewed changes

partitioned-heat-conduction/README.md Show resolved Hide resolved

partitioned-heat-conduction/openfoam-neumann/clean.sh Show resolved Hide resolved

Clean up cleaning files and add a solver cleaning script

7e18613

davidscn merged commit 4085a8e into precice:develop Jul 29, 2021

davidscn added a commit that referenced this pull request Jul 29, 2021

Add missing review changes supposed to be part of #223

1672e89

MakisH mentioned this pull request Jul 29, 2021

Add OpenFOAM partitioned heat equation #214

Closed

6 tasks

BenjaminRodenberg added this to the v202104.2.0 milestone Aug 4, 2021

davidscn deleted the partitioned-heat-OF branch September 23, 2021 14:34

Add partitioned-heat OpenFOAM participant with solver #223

Add partitioned-heat OpenFOAM participant with solver #223

Conversation

davidscn commented Jun 25, 2021 • edited Loading

MakisH commented Jun 25, 2021

davidscn commented Jun 25, 2021 • edited Loading

uekerman commented Jun 28, 2021 • edited Loading

uekerman commented Jun 28, 2021

MakisH commented Jun 28, 2021

uekerman commented Jun 28, 2021

MakisH commented Jun 28, 2021

davidscn commented Jun 29, 2021

davidscn commented Jul 13, 2021

uekerman commented Jul 14, 2021

davidscn commented Jul 14, 2021

uekerman commented Jul 14, 2021

davidscn commented Jul 14, 2021 • edited Loading

MakisH commented Jul 14, 2021

uekerman commented Jul 15, 2021

MakisH commented Jul 15, 2021

BenjaminRodenberg commented Jul 15, 2021

BenjaminRodenberg commented Jul 15, 2021

davidscn commented Jul 19, 2021

uekerman commented Jul 19, 2021

BenjaminRodenberg commented Jul 19, 2021

davidscn commented Jul 19, 2021

MakisH 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

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

davidscn commented Jul 20, 2021

Choose a reason for hiding this comment

davidscn commented Jul 20, 2021 • edited Loading

MakisH left a comment

Choose a reason for hiding this comment

davidscn commented Jul 29, 2021

davidscn commented Jun 25, 2021 •

edited

Loading

davidscn commented Jun 25, 2021 •

edited

Loading

uekerman commented Jun 28, 2021 •

edited

Loading

davidscn commented Jul 14, 2021 •

edited

Loading

davidscn commented Jul 20, 2021 •

edited

Loading