Epicure optimizations for mesh read-in#22
Conversation
…s3d-gpu into epicure_preprocess
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new NavierStokes UniformFlow test case to verify that the solver correctly maintains a uniform flow field. The test is designed to validate that a constant flow state remains unchanged throughout the simulation.
- Adds a new NavierStokes test case for uniform flow validation with HDF5 mesh support
- Updates CI workflows to enable testing on both
mainanddevelopbranches - Integrates the UniformFlow test into parallel and serial CI pipelines with HDF5 support
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Solver/test/NavierStokes/UniformFlow/UniformFlow_HDF5.control | New control file defining test parameters for uniform flow case with HDF5 mesh |
| Solver/test/NavierStokes/UniformFlow/SETUP/ProblemFile.f90 | Complete test implementation including initial conditions, boundary conditions, and validation logic |
| Solver/configure | Registers the new UniformFlow test case in the build configuration |
| .github/workflows/CI_serial_MU.yml | Enables CI triggers for the develop branch |
| .github/workflows/CI_parallel_NS.yml | Adds UniformFlow test to parallel NS CI pipeline with HDF5 support and standardizes build flags across tests |
| .github/workflows/CI_parallel_MU.yml | Enables CI triggers for the develop branch |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…HOPR hdf5 meshes Co-authored-by: zalbanob <171132623+zalbanob@users.noreply.github.com> Co-authored-by: Gonzalo Rubio <g.rubio@upm.es>
amrueda
changed the title
amrueda
changed the title
amrueda
changed the title
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| u = qq*cos(theta)*COS(phi) | ||
| v = qq*sin(theta)*COS(phi) | ||
| w = qq*SIN(phi) |
There was a problem hiding this comment.
Inconsistent case usage for trigonometric functions. Within the same expressions, cos/sin are used in lowercase while COS/SIN are used in uppercase. While Fortran is case-insensitive, consistent casing improves readability. Consider using either all lowercase (cos, sin) or all uppercase (COS, SIN) throughout.
| u = qq*cos(theta)*COS(phi) | |
| v = qq*sin(theta)*COS(phi) | |
| w = qq*SIN(phi) | |
| u = qq*cos(theta)*cos(phi) | |
| v = qq*sin(theta)*cos(phi) | |
| w = qq*sin(phi) |
| u = qq*cos(theta)*COS(phi) | ||
| v = qq*sin(theta)*COS(phi) | ||
| w = qq*SIN(phi) |
There was a problem hiding this comment.
Inconsistent case usage for trigonometric functions. Within the same expressions, cos/sin are used in lowercase while COS/SIN are used in uppercase. While Fortran is case-insensitive, consistent casing improves readability. Consider using either all lowercase (cos, sin) or all uppercase (COS, SIN) throughout.
| u = qq*cos(theta)*COS(phi) | |
| v = qq*sin(theta)*COS(phi) | |
| w = qq*SIN(phi) | |
| u = qq*cos(theta)*cos(phi) | |
| v = qq*sin(theta)*cos(phi) | |
| w = qq*sin(phi) |
| integer :: nvertex, i, j, k, ipoint, jpoint | ||
| integer :: idomain, npoints, ielem | ||
| logical :: isnewpoint, meshIsHOPR |
There was a problem hiding this comment.
The variables ipoint and isnewpoint are declared but never used in the refactored code. They should be removed to avoid compiler warnings about unused variables.
| integer :: nvertex, i, j, k, ipoint, jpoint | |
| integer :: idomain, npoints, ielem | |
| logical :: isnewpoint, meshIsHOPR | |
| integer :: nvertex, i, j, k, jpoint | |
| integer :: idomain, npoints, ielem | |
| logical :: meshIsHOPR |
| integer , intent(in) :: HOPRGlobalID | ||
| integer , intent(out) :: nodeID ! Node ID in HORSES3D! | ||
| !-------------------------------------------- | ||
| integer :: i ! Counter |
There was a problem hiding this comment.
The variable i is declared but never used in the refactored code. The old loop that used this variable has been replaced with a direct lookup. This unused variable declaration should be removed.
| integer :: i ! Counter |
| ! | ||
| ! The procedures, *even if empty* that must be defined are | ||
| ! | ||
| ! UserDefinedSetUp |
There was a problem hiding this comment.
The comment lists "UserDefinedSetUp" as a required procedure, but the actual subroutine implemented is "UserDefinedStartup" (line 20). The comment should be corrected to match the implementation.
| ! UserDefinedSetUp | |
| ! UserDefinedStartup |
| QExpected(:,i,j,k) = Q | ||
| end do; end do; end do | ||
| end associate | ||
| maxError = MAXVAL(ABS(QExpected - mesh % elements(eID) % storage % Q)) |
There was a problem hiding this comment.
The maxError calculation appears to have a logic issue. The QExpected array is populated inside the element loop (lines 334-353), and then maxError is computed for each element individually (line 354). However, maxError is not accumulated across elements - it's simply overwritten for each element. This means only the error from the last element is retained. Consider either: 1) moving the maxError calculation outside the loop and comparing all elements at once, or 2) using maxError = MAX(maxError, MAXVAL(ABS(QExpected - mesh % elements(eID) % storage % Q))) to accumulate the maximum across all elements.
| maxError = MAXVAL(ABS(QExpected - mesh % elements(eID) % storage % Q)) | |
| maxError = MAX(maxError, MAXVAL(ABS(QExpected - mesh % elements(eID) % storage % Q))) |
loganoz
left a comment
loganoz
left a comment
There was a problem hiding this comment.
I reviewed the changes on the workflows and test cases and they look fine to me. About the changes in the HOPR reader and Mesh partitioning, are these only epicure cherry picked by you right? I understand if they pass the test case you added they should be fine. Not sure about the efficiency, that we might need to check.
Can you write down somewhere what is already implemented from their PR and what's missing? We will forget if we don't do it now.
This PR adds a serial and parallel uniform-flow test for HOPR HDF5 meshes and incorporates the Epicure BSC optimizations for mesh read-in (all Epicure BSC optimizations have been added in #8). Some fixes were required to allow the mesh read-in optimizations to compile with support for HDF5 meshes.