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 Fortran elastic-tube-1d solvers (precice Fortran module) #607

Merged
merged 37 commits into from
Jan 29, 2025

Conversation

YonatanGM
Copy link
Contributor

@YonatanGM YonatanGM commented Jan 8, 2025

This PR adds Fortran solvers for the elastic-tube-1d case using the fortran module (precice.f90). I also opened PR #606 that doesn't rely on this module.

The run.sh script downloads precice.f90 from the Fortran module repository to get the latest version and the file gets compiled with the solvers source files using CMake. Since the module is just a wrapper around the precicef functions in the library, which are updated with preCICE release, I imagine it will stay compatible with the precice version used in the solvers (as long as the function names stay consistent)

LAPACK is needed for the fluid solver (same as the C++ case). Just run ./run.sh to build and execute via CMake. The plotting scripts (plot-diameter.sh and plot-all.sh) are updated to show results for the Fortran case. The precommit yaml didnt have hooks for Fortran files, so I maunally formatted them using fprettify.

Results look consistent with the other solvers. I checked the VTK output, and the numbers match up to 5 decimal places on the last timestep. Also tested running the fortran case against the other solves and that seems to work fine too.

tutorials-elastic-tube-1d-all
Figure-fortran
Figure-python

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.
  • Update the image used in readme showing diameter and pressure of running all solvers against cpp to inlcude fortran case

@YonatanGM YonatanGM marked this pull request as draft January 8, 2025 16:16
@YonatanGM YonatanGM marked this pull request as ready for review January 8, 2025 16:20
@YonatanGM YonatanGM mentioned this pull request Jan 8, 2025
3 tasks
@MakisH MakisH self-requested a review January 8, 2025 17:18
@MakisH MakisH self-assigned this Jan 8, 2025
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.

Hi @YonatanGM,

wearing my preCICE hat here 🤠 (it is a bit unfortunate that I am also supervising your SSE project, maybe @fsimonis can control my conflict of interest).

With a first look, I think we could keep both contributions, since we support both ways of using preCICE via Fortran. Let's start with this one, and at the same time port any changes to the other one as well (should not be too many). Not sure yet how we could reduce duplication, but I have the impression the simplest would be to keep both, in different directories (fluid-fortran and fluid-fortran-module).

I had a first look and I ran the code, mixing languages to check for potential incompatibilities. I arrive at the same results.

tutorials-elastic-tube-1d-all

In terms of the solver source code, I have only reviewed the FluidComputeSolution.f90 so far, as well as the shell scripts and CMakeLists.txt. I will check the rest of the Fortran files during the day.

elastic-tube-1d/fluid-fortran/CMakeLists.txt Outdated Show resolved Hide resolved
elastic-tube-1d/fluid-fortran/CMakeLists.txt Outdated Show resolved Hide resolved
elastic-tube-1d/fluid-fortran/run.sh Outdated Show resolved Hide resolved
elastic-tube-1d/fluid-fortran/src/FluidComputeSolution.f90 Outdated Show resolved Hide resolved
elastic-tube-1d/fluid-fortran/src/FluidComputeSolution.f90 Outdated Show resolved Hide resolved
elastic-tube-1d/fluid-fortran/src/FluidComputeSolution.f90 Outdated Show resolved Hide resolved
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.

Some of my comments were still somehow pending publishing. @YonatanGM let me know if everything is clear.

elastic-tube-1d/fluid-fortran/src/FluidSolver.f90 Outdated Show resolved Hide resolved
elastic-tube-1d/fluid-fortran/src/FluidSolver.f90 Outdated Show resolved Hide resolved
elastic-tube-1d/fluid-fortran/src/utilities.f90 Outdated Show resolved Hide resolved
@YonatanGM
Copy link
Contributor Author

YonatanGM commented Jan 23, 2025

Some of my comments were still somehow pending publishing. @YonatanGM let me know if everything is clear.

Hi @MakisH, Thanks for the comments!! I will address them later today or tomorrow.

@YonatanGM YonatanGM force-pushed the elastic-tube-1d-fortran-module branch from 15a9a88 to b0d4593 Compare January 27, 2025 05:52
@YonatanGM
Copy link
Contributor Author

YonatanGM commented Jan 27, 2025

Hi @MakisH, I addressed your suggestions! I renamed the folder to fluid/solid-fortran-module. PR #606 uses fluid/solid-fortran.
One problem is the plot-all.sh and plot-diameter.sh scripts in this PR still reference the fluid-fortranfolder (which is not there anymore), so if you try to run those scripts they will fail. But once both PRs are merged there should no issue. The results from both PRs are the same, so I though it makes sense to just plot one of them.

@YonatanGM
Copy link
Contributor Author

YonatanGM commented Jan 27, 2025

@MakisH I noticed there are no precice finalize calls in the C++ and python versions. Could that be problematic?

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 very much for the prompt and very clean updates. Looks already very good! Just a couple of tiny points, mostly replying to your questions.

elastic-tube-1d/plot-all.sh Outdated Show resolved Hide resolved
elastic-tube-1d/fluid-fortran-module/output/.keepme Outdated Show resolved Hide resolved
@MakisH
Copy link
Member

MakisH commented Jan 27, 2025

@MakisH I noticed there are no precice finalize calls in the C++ and python versions. Could that be problematic?

Good observation, thanks for asking!

In the C++ API, the finalize() is called in the destructor of the Participant class, so it is called automatically when the program exits (i.e., when the object goes out of scope). This is a good practice, which has the strange name "Resource Acquisition Is Initialization (RAII)". I assume the Python bindings implement the same.

In C and Fortran, the whole paradigm is procedural, so we need to call this explicitly.

tl;dr: All good.

@MakisH
Copy link
Member

MakisH commented Jan 27, 2025

Also: Don't forget the changelog entry (in both PRs).

@MakisH
Copy link
Member

MakisH commented Jan 29, 2025

The code works. Still pending:

  • removing the output/.keepme file and the output/ directory (I tested it, and it works without) create the directory in the run script.
  • adding a changelog file changelog-entries/607.md

(putting my SSE hat on: these are trivial, but still have some learning significance)

@YonatanGM
Copy link
Contributor Author

@MakisH With both directory and file removed, it didnt work for me.
Getting errors like this: Error: Unable to open file ./output/out_fluid_99.vtk
I think we have to keep the folder or create it first if it doesnt exist when writing the vtk files. What do you think?

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 the very clean contribution and the very prompt and clear answers, @YonatanGM! I am merging this and I will merge #606 next.

@MakisH MakisH merged commit 5373c4d into precice:develop Jan 29, 2025
1 check passed
@MakisH
Copy link
Member

MakisH commented Jan 29, 2025

After merging, I noticed one small thing that I would have done differently and slipped my review: the precice.f90 is downloaded into src/. I would normally put it under thirdparty/ (or even get the repository as a git module), to keep it clearly separated.

If you want to contribute this as well, that would be great, but I can also do it later.

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