Follow standard CMake layout#214
Conversation
|
@tpadioleau can you have a look at this please to see if it aligns with what you had in mind for linking to Gyselalib++? |
69ca3ed to
7392ea6
Compare
6a681a8 to
6e94e02
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #214 +/- ##
=======================================
Coverage 95.73% 95.73%
=======================================
Files 79 79
Lines 8622 8622
=======================================
Hits 8254 8254
Misses 368 368 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
tpadioleau
left a comment
There was a problem hiding this comment.
I think this is an improvement, my main concern is whether we should install files related to InputFunctions ?
There was a problem hiding this comment.
Is this METIS from https://github.com/KarypisLab/METIS ? I see it is based on CMake, it does not generate its own Config files ?
Does not need to be handled in the PR.
There was a problem hiding this comment.
I didn't check, I just reorganised what was already in the existing CMake files. Probably a question for @mknaranja
There was a problem hiding this comment.
more a comment than a full answer: Yes, this is METIS from Karypis, it is used for mesh / graph partitioning which is used in direct solvers such as MUMPS. But: I couldn't tell you anything about the technical config file question, I never looked at it at that level.
There was a problem hiding this comment.
I never tested Mumps without Metis, but Mumps could be used without it by setting ICNTL(7)=7.
https://github.com/SciCompMod/GMGPolar/blob/main/src/LinearAlgebra/Solvers/coo_mumps_solver.cpp#L107
There was a problem hiding this comment.
I think MUMPS can also use other graph partitioners such as p4est but I think that does only yield other problems. Graph partitioning is used to reduce fill-in. I would suggest to keep METIS if MUMPS is used. From my point of view, METIS is a minimal dependency as it is used in millions of numerical linear algebra (+ HPC) applications.
| install(TARGETS | ||
| InputFunctions | ||
| InputFunctions_BoundaryConditions | ||
| InputFunctions_DensityProfileCoefficients | ||
| InputFunctions_DomainGeometry | ||
| InputFunctions_ExactSolution | ||
| InputFunctions_SourceTerms | ||
| EXPORT GMGPolarTargets | ||
| ARCHIVE DESTINATION lib | ||
| ) |
There was a problem hiding this comment.
Do they actually need to be installed ?
There was a problem hiding this comment.
I'm not sure. This is a question for @mknaranja . It depends on what you see as being the main functionality of the library.
I would lean towards them not needing to be installed. But if they are I would probably use them for testing GMGPolar vs other solvers
There was a problem hiding this comment.
I can see a world where someone might want to use e.g. the domain geometry for a problem. I don't see any harm in installing them. Unless you see a problem?
There was a problem hiding this comment.
No strong concern, it is just usually easier to add a new feature than remove it. So if we were not sure it is public, we could have delayed its installation in an other release. But if these functions make sense, we keep them.
Co-authored-by: Thomas Padioleau <thomas.padioleau@cea.fr>
HenrZu
left a comment
There was a problem hiding this comment.
@mknaranja requested me to have a look at this. Looks great :) The structure is much cleaner. I left a few minor comments (most are questions as im not really familiar with the structure).
Co-authored-by: Thomas Padioleau <thomas.padioleau@cea.fr>
|
FYI, I started a draft of the Spack package, gyselax/spack@e4fbf54 You can test it with a Spack environment: # gmgpolar-spack.yaml
spack:
concretizer:
unify: true
specs:
- 'cmake'
- 'gmgpolar@git.ebourne_cleanup_cmake=main'
repos:
patches:
git: https://github.com/gyselax/spack.git
branch: releases/v2025.11
builtin:
branch: releases/v2025.11
view: trueOnce you have downloaded Spack and activated it, see https://spack.readthedocs.io/en/latest/getting_started.html#getting-started |
|
Hello, what remains to do for this PR ? |
I am still waiting for a reply on these threads: from @mknaranja or @HenrZu to determine what (if anything) remains to be done |
Clean up CMake files to follow standard practice. In particular:
find_packagecalls are moved to the rootCMakeLists.txtso dependencies are easy to locatecmake/FindX.cmakefiles so they can be located via a standardfind_packagecallInputFunctions(which is only required for testing and for the CLI) is removed from theGMGPolarLibtargetGMGPolarInterfaceis added to group example input functions and other tools required for the command line interface.if(OpenMP_CXX_FOUND)is removed as OpenMP is a required dependency so this should always be Trueinclude/folder is included via the compile commandfind_package(GMGPolar)Merge Request - GuideLine Checklist
Guideline to check code before resolve WIP and approval, respectively.
As many checkboxes as possible should be ticked.
Checks by code author:
Always to be checked:
If functions were changed or functionality was added:
If new functionality was added:
If new third party software is used:
If new mathematical methods or epidemiological terms are used:
Checks by code reviewer(s):