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.

Sign up for GitHub

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

Jump to bottom

PETSc XZHermiteSpline #2917

Open
wants to merge 14 commits into
base: next
Choose a base branch
from
Open

PETSc XZHermiteSpline #2917

wants to merge 14 commits into from

Conversation

ZedThree
Copy link
Member

Continues #2858 pulling out a separate class for PetscXZHermiteSpline that was started in #2651

This does have a working implementation, but I've tried to switch it the existing PetscMatrix wrapper, and unfortunately I've gone wrong somewhere -- probably at the boundaries.

Double unfortunately, I really don't have much time to spend digging into this. @dschwoerer Maybe you want to take a look?

bendudson and others added 10 commits February 3, 2024 11:14
@bendudson
Simplify XZInterpolation class
6ee9f74 
Reduce duplication by moving some overloads to the base class.
@bendudson
Moving registration to header
936cd55 
Optimised build was omitting XZInterpolation factory registrations.
@ZedThree
Merge branch 'next' into petsc-interpolation
2dc3967 
* next: (194 commits)
  invert_laplace: Fix outer boundary metrics
  Apply clang-format changes
  Apply clang-format changes
  Apply suggestions from code review
  Fix missed adios
  Prefer ADIOS2 over ADIOS in more cases
  Update tests/unit/test_extras.hxx
  Apply clang-format changes
  Silence warning about assignment in `if` statement
  Fix some short identifiers
  Make a couple of implicit casts explicit
  CI: Suppress some warnings about short identifiers
  Move some static functions to anonymous namespace
  CI: Remove deprecated clang-tidy option
  Include header for `BoutReal`; remove unused header
  Apply clang-format
  Add options to set Butcher tables by name
  Simplify SUNDIALS cmake
  Switch to [[maybe_unused]]
  Apply suggestions from code review
  ...
@ZedThree
Make interpolation registration variables const inline
0cdb962
@ZedThree
Fix issue with overload hiding base virtual functions
86ee8be
@ZedThree
Make lagrange_4pt interpolation function a static free function
cb8b550
@ZedThree
Add PetscXZHermiteSpline that can be split in x direction
b9cbb52 
Based on @dschwoerer's implementation in #2651
@ZedThree
WIP: Switch PetscXZHermiteSpline to use PetscMatrix for weights
183c7a2 
Currently gives pretty bad results, not sure why
@ZedThree
Fix headers when building without PETSc
39bef1c
@ZedThree @github-actions
Apply clang-format changes
654f605
github-actions[bot]
github-actions bot reviewed May 24, 2024
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/mesh/interpolation/petsc_hermite_spline_xz.cxx Outdated Show resolved Hide resolved
src/mesh/interpolation/petsc_hermite_spline_xz.cxx Outdated Show resolved Hide resolved
src/mesh/interpolation/petsc_hermite_spline_xz.cxx Outdated Show resolved Hide resolved
src/mesh/interpolation/petsc_hermite_spline_xz.cxx Outdated Show resolved Hide resolved
src/mesh/interpolation/petsc_hermite_spline_xz.cxx Outdated Show resolved Hide resolved
src/mesh/interpolation/petsc_hermite_spline_xz.cxx Outdated Show resolved Hide resolved
src/mesh/interpolation/petsc_hermite_spline_xz.cxx Outdated Show resolved Hide resolved
src/mesh/interpolation/petsc_hermite_spline_xz.cxx Outdated Show resolved Hide resolved
src/mesh/interpolation/petsc_hermite_spline_xz.cxx Outdated Show resolved Hide resolved
tests/integrated/test-interpolate/test_interpolate.cxx Outdated Show resolved Hide resolved
@boutproject boutproject deleted a comment from github-actions bot May 30, 2024
@ZedThree ZedThree marked this pull request as ready for review May 30, 2024 16:57
@ZedThree
Copy link
Member Author

Ok, I have managed to convert this to use the PetscMatrix interface. I need to spend a lot of time faffing about with GlobalIndexer and realised the source of my pain: it can only convert indices that are both local to and owned by the current process. For the interpolation here, we want to deal with indices that are on other processes. And because PetscMatrix wants to convert local indices to global ones, we need to first convert the global index of the (x, z) offsets into local ones, and then convert those to global indices.

So I've implemented a new GlobalIndexer subclass that will probably only work for very simple parallel decompositions, but can convert e.g. negative local indices into global ones. That gets an index converter that is actually pretty straightforward and can be used with PetscMatrix, which then allows a pretty clean implementation in our new interpolation!

@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@ZedThree
Remove recursive check
b7f5b94 
Although `getGlobal` is only valid for local indices, unfortunately
`isLocal` calls `getGlobal`, so we can't actually check!
@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@ZedThree ZedThree changed the title WIP: PETSc XZHermiteSpline PETSc XZHermiteSpline May 31, 2024
@ZedThree ZedThree requested a review from dschwoerer October 23, 2024 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@dschwoerer dschwoerer Awaiting requested review from dschwoerer

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ZedThree @bendudson