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

Follow naming convention in partitioned heat conduction - FEniCS #459

Merged

Conversation

BenjaminRodenberg
Copy link
Member

Fixed incorrect order for OpenFOAM

Updated structure to the one provided here for FEniCS.

Other cases are still missing.

@BenjaminRodenberg BenjaminRodenberg added the good first issue Good for newcomers label Feb 4, 2024
@BenjaminRodenberg BenjaminRodenberg changed the base branch from master to develop February 4, 2024 19:17
@BenjaminRodenberg
Copy link
Member Author

@MakisH I think the order in the OpenFOAM cases was incorrect. Fixed it. For the other cases we also need an update, but maybe this is also a good first issue? Should be relatively simple to do and is still a valuable contribution.

Comment on lines +102 to 111
precice = Adapter(adapter_config_filename="precice-adapter-config.json")

if problem is ProblemType.DIRICHLET:
precice = Adapter(adapter_config_filename="precice-adapter-config-D.json")
precice.initialize(coupling_boundary, read_function_space=V, write_object=f_N_function)
precice_dt = precice.get_max_time_step_size()
elif problem is ProblemType.NEUMANN:
precice = Adapter(adapter_config_filename="precice-adapter-config-N.json")
precice.initialize(coupling_boundary, read_function_space=W, write_object=u_D_function)
precice_dt = precice.get_max_time_step_size()

precice_dt = precice.get_max_time_step_size()
dt = Constant(0)
dt.assign(np.min([fenics_dt, precice_dt]))
Copy link
Member Author

Choose a reason for hiding this comment

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

I like the fact that the new structure simplifies the mapping of solver to config file 👍

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.

Thank you for fixing this! I will update the structure specification as well.

The openfoam-solver was there before we come up with some kind of naming convention for solver directories. Thank you for fixing that as well!

Changes look good. This will not create conflicts with any other PR, right?

edit: Not an issue (but PR), therefore also not a good-first-issue.

@MakisH MakisH removed the good first issue Good for newcomers label Feb 5, 2024
@BenjaminRodenberg
Copy link
Member Author

Changes look good. This will not create conflicts with any other PR, right?

No. Should be fine.

edit: Not an issue (but PR), therefore also not a good-first-issue.

Do you think we can find somebody to take care about the rest? It's a nice-to-have and currently don't want to prioritize this. So let's either take care about all cases soon or directly merge and take care about the rest later (feel free to merge).

@MakisH
Copy link
Member

MakisH commented Feb 5, 2024

edit: Not an issue (but PR), therefore also not a good-first-issue.

Do you think we can find somebody to take care about the rest? It's a nice-to-have and currently don't want to prioritize this. So let's either take care about all cases soon or directly merge and take care about the rest later (feel free to merge).

Let's open an issue for that, with a checklist of which cases need to be restructured.

I would already merge this one, to let other PRs come as well.

MakisH added a commit to precice/precice.github.io that referenced this pull request Feb 7, 2024
As discussed in precice/tutorials#228 and partially implemented in precice/tutorials#459

Also updates minor aspects in the whole page (and introduces an intentional typo).
@MakisH MakisH marked this pull request as ready for review February 7, 2024 15:26
@MakisH
Copy link
Member

MakisH commented Feb 7, 2024

I consider this ready to merge. I prepared an overview regarding the rest of the cases here: #461

@MakisH MakisH changed the title Follow naming convention with partitioned heat conduction Follow naming convention in partitioned heat conduction - FEniCS Feb 7, 2024
@MakisH MakisH merged commit fca2363 into precice:develop Feb 7, 2024
MakisH added a commit to MakisH/tutorials that referenced this pull request Mar 10, 2024
Unintentionally deleted in precice#459 as a result of a wrong gitignore
@davidscn
Copy link
Member

With the changes applied here and in #477 the solver-openfoam doesn't compile anymore. Did anyone test this before merging or is it just a local problem of my OpenFOAM installation? (tried with 2306 and 2012.

MakisH added a commit that referenced this pull request Mar 25, 2024
@MakisH
Copy link
Member

MakisH commented Mar 25, 2024

I don't see how it could be at all an issue of the renaming, but it indeed does not compile for me anymore:

~/repos/precice/tutorials/partitioned-heat-conduction/solver-openfoam [develop]$ wmake
Making dependencies: heatTransfer.C
g++ -std=c++14 -m64 -pthread -DOPENFOAM=2312 -DWM_DP -DWM_LABEL_SIZE=32 -Wall -Wextra -Wold-style-cast -Wnon-virtual-dtor -Wno-unused-parameter -Wno-invalid-offsetof -Wno-attributes -Wno-unknown-pragmas -O3  -DNoRepository -ftemplate-depth-100  -I/usr/lib/openfoam/openfoam2312/src/finiteVolume/lnInclude -I/usr/lib/openfoam/openfoam2312/src/meshTools/lnInclude -iquote. -IlnInclude -I/usr/lib/openfoam/openfoam2312/src/OpenFOAM/lnInclude -I/usr/lib/openfoam/openfoam2312/src/OSspecific/POSIX/lnInclude   -fPIC -c heatTransfer.C -o Make/linux64GccDPInt32Opt/heatTransfer.o
In file included from heatTransfer.C:89:
/usr/lib/openfoam/openfoam2312/src/OpenFOAM/lnInclude/createMesh.H: In function ‘int main(int, char**)’:
/usr/lib/openfoam/openfoam2312/src/OpenFOAM/lnInclude/createMesh.H:37:5: error: ‘args’ was not declared in this scope; did you mean ‘argc’?
   37 |     args.getOrDefault<word>("region", Foam::polyMesh::defaultRegion)
      |     ^~~~
      |     argc
/usr/lib/openfoam/openfoam2312/src/OpenFOAM/lnInclude/createMesh.H:37:27: error: expected primary-expression before ‘>’ token
   37 |     args.getOrDefault<word>("region", Foam::polyMesh::defaultRegion)
      |                           ^
/usr/lib/openfoam/openfoam2312/src/OpenFOAM/lnInclude/createMesh.H:37:29: warning: left operand of comma operator has no effect [-Wunused-value]
   37 |     args.getOrDefault<word>("region", Foam::polyMesh::defaultRegion)
      |                             ^~~~~~~~
/usr/lib/openfoam/openfoam2312/src/OpenFOAM/lnInclude/createMesh.H:51:50: error: ‘runTime’ was not declared in this scope
   51 |         new Foam::simplifiedMeshes::columnFvMesh(runTime, regionName)
      |                                                  ^~~~~~~
/usr/lib/openfoam/openfoam2312/src/OpenFOAM/lnInclude/createMesh.H:81:37: error: ‘runTime’ was not declared in this scope
   81 |     Foam::Info << " for time = " << runTime.timeName() << Foam::nl;
      |                                     ^~~~~~~
make: *** [/usr/lib/openfoam/openfoam2312/wmake/rules/General/transform:38: Make/linux64GccDPInt32Opt/heatTransfer.o] Error 1

I don't remember when was the last time I tested this case, probably not when merging this, given that I needed to restore the Make/ directory in #477.

@MakisH
Copy link
Member

MakisH commented Mar 25, 2024

Irrelevant to this PR, but to #468. Fixed in #499.

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.

3 participants