Skip to content

Add GMGPolar as a possible IPolarPoissonLikeSolver#644

Open
EmilyBourne wants to merge 22 commits into
develfrom
ebourne_gmgpolar
Open

Add GMGPolar as a possible IPolarPoissonLikeSolver#644
EmilyBourne wants to merge 22 commits into
develfrom
ebourne_gmgpolar

Conversation

@EmilyBourne
Copy link
Copy Markdown
Member

Add GMGPolar as a possible IPolarPoissonLikeSolver.

SciCompMod/GMGPolar#214 should be merged before merging this PR


Please complete the checklist to ensure that all tasks are completed before marking your pull request as ready for review.

All Submissions

  • Have you ensured that all lines changed in this PR are justified by a comment found in the description ?
  • Have you updated the CHANGELOG.md ?
  • Have you linked any issues that should be closed when this PR is merged (using closing keywords) ?
  • Have you checked that the AUTHORS file is up to date ?
  • Have you checked that the copyright information in the LICENCE file is up to date (including dates) ?
  • Do you follow the conventions specified in our coding standards ?

New Feature Submissions

  • Have you added tests for the new functionalities ?
  • Have you documented the new functionalities:
    • API documentation describing the available methods, when each should be used and how to use them ?
    • User-friendly documentation in README files (which may link to the API documentation).
    • If the new functionality is non-trivial to use, provide a tutorial or example ? (optional)

Changes to Existing Features

  • Have you checked that existing tests cover all code after the changes ?
  • Have you checked that existing tests are still passing ?
  • Have you checked that the existing documentation is still accurate (API and README files) ?

Changes to the CI

  • Have you made the same changes to both the GitHub CI and the GitLab CI (for the private fork) ?

@EmilyBourne EmilyBourne added the Do not merge Label for a PR that should not be merged even if it appears to be ready label Apr 21, 2026
@github-actions
Copy link
Copy Markdown

This PR is failing tests so it has been put back into draft. Please remove the draft status when the tests pass.

@github-actions github-actions Bot marked this pull request as draft April 21, 2026 12:59
@EmilyBourne EmilyBourne marked this pull request as ready for review April 21, 2026 13:50
@github-actions
Copy link
Copy Markdown

This PR is failing tests so it has been put back into draft. Please remove the draft status when the tests pass.

@github-actions github-actions Bot marked this pull request as draft April 21, 2026 14:00
@EmilyBourne EmilyBourne marked this pull request as ready for review April 21, 2026 15:24
@EmilyBourne EmilyBourne temporarily deployed to GitLab GPU trigger April 21, 2026 15:25 — with GitHub Actions Inactive
@github-actions github-actions Bot added the Ready to review Label to be automatically added to a PR when it is ready to be reviewed label Apr 21, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 21, 2026

Codecov Report

❌ Patch coverage is 92.20779% with 6 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/pde_solvers/gmg_polar_poisson_like_solver.hpp 92.20% 6 Missing ⚠️

📢 Thoughts on this report? Let us know!

Comment thread CMakeLists.txt
set(GMGPOLAR_BUILD_TESTS OFF CACHE BOOL "")
add_subdirectory("vendor/GMGPolar/" "GMGPolar" EXCLUDE_FROM_ALL SYSTEM)
# Suppress -Werror=unused-variable inherited via CMAKE_CXX_FLAGS_INIT so that GMGPolar warnings don't break the build.
target_compile_options(GMGPolarLib PRIVATE -Wno-error=unused-variable)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Another reason I don't like vendoring :)

Comment thread CMakeLists.txt Outdated
find_package(Ginkgo 1.8 REQUIRED)

set(GMGPOLAR_BUILD_TESTS OFF CACHE BOOL "")
add_subdirectory("vendor/GMGPolar/" "GMGPolar" EXCLUDE_FROM_ALL SYSTEM)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
add_subdirectory("vendor/GMGPolar/" "GMGPolar" EXCLUDE_FROM_ALL SYSTEM)
add_subdirectory(vendor/GMGPolar EXCLUDE_FROM_ALL SYSTEM)

Do we have a reason to exclude from all ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't want to compile the executables or the test/interface library. I am only interested in one library from the folder (GMGPolarLib)

Is there a reason to not use EXCLUDE_FROM_ALL?
It doesn't stop anything from being compiled it just ensures that the only things that are compiled are the libs that are used

Comment thread CMakeLists.txt

find_package(Ginkgo 1.8 REQUIRED)

set(GMGPOLAR_BUILD_TESTS OFF CACHE BOOL "")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One reason I don't like vendoring

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Actually maybe we don't need this if we're using EXCLUDE_FROM_ALL 🤔

Comment thread src/pde_solvers/CMakeLists.txt Outdated
gslx::data_types
gslx::matrix_tools
gslx::utils
GMGPolarLib
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should push for an alias before merging.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is why I asked for your opinion on the CMake PR on the GMGPolar side

@github-actions github-actions Bot removed the Ready to review Label to be automatically added to a PR when it is ready to be reviewed label Apr 21, 2026
@github-actions github-actions Bot marked this pull request as draft April 21, 2026 16:26
Comment thread src/pde_solvers/CMakeLists.txt Outdated
gslx::data_types
gslx::matrix_tools
gslx::utils
GMGPolarLib
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
GMGPolarLib
GMGPolar::GMGPolarLib

@EmilyBourne EmilyBourne marked this pull request as ready for review April 22, 2026 08:54
@github-actions
Copy link
Copy Markdown

This PR is failing tests so it has been put back into draft. Please remove the draft status when the tests pass.

@github-actions github-actions Bot marked this pull request as draft April 22, 2026 08:55
@EmilyBourne EmilyBourne marked this pull request as ready for review April 22, 2026 09:05
@github-actions github-actions Bot added the Ready to review Label to be automatically added to a PR when it is ready to be reviewed label Apr 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Do not merge Label for a PR that should not be merged even if it appears to be ready Ready to review Label to be automatically added to a PR when it is ready to be reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants