Skip to content

Calculate Jacobian coloring using PetscOperator matrices#3350

Open
bendudson wants to merge 20 commits into
nextfrom
petsc-operators-snes
Open

Calculate Jacobian coloring using PetscOperator matrices#3350
bendudson wants to merge 20 commits into
nextfrom
petsc-operators-snes

Conversation

@bendudson

Copy link
Copy Markdown
Contributor

Builds on #3330 , using PetscOperator matrices to infer connectivity between cells.

The aim is to accelerate transport simulations with FCI by inferring the sparsity pattern of the Jacobian in SNESSolver and PetscSolver. This will enable coloring to efficiently calculate finite difference Jacobian approximations for LU-like preconditioners. This is the method used for structured grid (e.g. Tokamak) simulations, that so far has not been possible with FCI.

This function should create an index set of the evolving cells.
It will be used to extract the part of the PetscOperator that
represents coupling between evolving cells.

Implementation to follow.
Creates an index set of global PETSc rows that correspond
to evolving cells.

Passes unit tests.
Method that will extract the part of a PetscCellOperator that
couples evolving cells to other evolving cells.

Dummy implementation with failing tests.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

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

// Collect global PETSc indices in mapOwnedInteriorCells order.
// Reserve the known count up front to avoid reallocation.
std::vector<PetscInt> indices;
indices.reserve(static_cast<std::size_t>(evolving_region.size()));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "std::size_t" is directly included [misc-include-cleaner]

src/mesh/petsc_operators.cxx:1:

+ #include <cstddef>

mapOwnedInteriorCells(
[&](PetscInt row, const Ind3D& /*i*/, int /*stored*/) { indices.push_back(row); });

IS is;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: variable 'is' is not initialized [cppcoreguidelines-init-variables]

Suggested change
IS is;
IS is = nullptr;

const PetscOperator<CellSpaceTag, CellSpaceTag>& op) const {
IS is = makeEvolvingIS();

Mat sub;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: variable 'sub' is not initialized [cppcoreguidelines-init-variables]

Suggested change
Mat sub;
Mat sub = nullptr;

Checks the global ordering of cells for consistency as number
of processors and nxpe is varied.
Include guards, header includes.
This function will add non-zeros to a Jacobian matrix, based on
a submatrix from a PetscCellOperator.

Dummy implementation so unit tests fail.
Sets a sparsity pattern in a given Jacobian matrix, based
on an operator matrix and the variable indices.

Passes unit tests.
Should apply the connectivity pattern to all pairs of variables
in the system Jacobian.
Applies the same connectivity in `mat` to all pairs of variables in
`J`. Passes unit tests.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

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


void addOperatorSparsity(Mat Jfd, Mat sub, int out_var, int in_var) {
// Infer nvars from global sizes
PetscInt jfd_global{0}, sub_global{0};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]

Suggested change
PetscInt jfd_global{0}, sub_global{0};
PetscInt jfd_global{0};
PetscInt sub_global{0};

ASSERT1(in_var >= 0 && in_var < static_cast<int>(nvars));

// Iterate over locally owned rows of sub and insert into Jfd
PetscInt rstart{0}, rend{0};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]

Suggested change
PetscInt rstart{0}, rend{0};
PetscInt rstart{0};
PetscInt rend{0};

}

void addOperatorSparsity(Mat Jfd, Mat sub) {
PetscInt jfd_global{0}, sub_global{0};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]

Suggested change
PetscInt jfd_global{0}, sub_global{0};
PetscInt jfd_global{0};
PetscInt sub_global{0};

#include <bout/petsc_operators.hxx>
#include <bout/region.hxx>
#include <bout/utils.hxx>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: included header utils.hxx is not used directly [misc-include-cleaner]

Suggested change

MatDestroy(&dummy);

// ── Compare traversal orderings ───────────────────────────────────────────
// Walk mapOwnedInteriorCells and RGN_NOBNDRY in parallel, checking that

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: variable 'Istart' is not initialized [cppcoreguidelines-init-variables]

RMINE);
                                                                      ^

this fix will not be applied because it overlaps with another fix


// ── Compare traversal orderings ───────────────────────────────────────────
// Walk mapOwnedInteriorCells and RGN_NOBNDRY in parallel, checking that
// the local offset produced by each is identical for every cell.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]

Suggested change
// the local offset produced by each is identical for every cell.
RMINE);
t, Iend;PetscInt Istart;
PetscInt Iend;


// ── Compare traversal orderings ───────────────────────────────────────────
// Walk mapOwnedInteriorCells and RGN_NOBNDRY in parallel, checking that
// the local offset produced by each is identical for every cell.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: variable 'Iend' is not initialized [cppcoreguidelines-init-variables]

t, Iend;
   ^

this fix will not be applied because it overlaps with another fix

}

if (my_rank == 0) {
output.write("total_mismatches={}\n", total_mismatches);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: variable 'my_rank' is not initialized [cppcoreguidelines-init-variables]

tests/integrated/test-petsc-ordering/test_petsc_ordering.cxx:201:

- k;
+ k = 0;

Test passes locally but fails in CI

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread src/mesh/petsc_operators.cxx Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread examples/fci-operators/fci_operators_example.cxx
Comment thread examples/fci-operators/fci_operators_example.cxx
}
}
}
// Communicate so guard cells are filled (needed by some mesh operations)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]

Suggested change
// Communicate so guard cells are filled (needed by some mesh operations)
ell_number(x, y, z) = (((global_x * ngy) + global_y) * ngz) + global_z;

MatDestroy(&dummy);

// ── Compare traversal orderings ───────────────────────────────────────────
// Walk mapOwnedInteriorCells and RGN_NOBNDRY in parallel, checking that

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: variable 'Iend' is not initialized [cppcoreguidelines-init-variables]

MatSetType(dummy, MATMPIAIJ);
                                                                        ^

this fix will not be applied because it overlaps with another fix

MatDestroy(&dummy);

// ── Compare traversal orderings ───────────────────────────────────────────
// Walk mapOwnedInteriorCells and RGN_NOBNDRY in parallel, checking that

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: variable 'Istart' is not initialized [cppcoreguidelines-init-variables]

MatSetType(dummy, MATMPIAIJ);
                                                                ^

this fix will not be applied because it overlaps with another fix


// ── Compare traversal orderings ───────────────────────────────────────────
// Walk mapOwnedInteriorCells and RGN_NOBNDRY in parallel, checking that
// the local offset produced by each is identical for every cell.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]

Suggested change
// the local offset produced by each is identical for every cell.
MatSetType(dummy, MATMPIAIJ);PetscInt Istart;
PetscInt Iend;

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

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

// Collect global PETSc indices in mapOwnedInteriorCells order.
// Reserve the known count up front to avoid reallocation.
std::vector<PetscInt> indices;
indices.reserve(static_cast<std::size_t>(evolving_region.size()));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "std::size_t" is directly included [misc-include-cleaner]

src/mesh/petsc_operators.cxx:5:

+ #include <cstddef>

Field2D expressions need to be evaluated before assignment to Field3D.
Handle the case where `source` is `nullptr`.
Assumes no evolving Field2D or evolving boundary cells.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread src/mesh/petsc_operators.cxx
Base automatically changed from petsc-operators to next June 30, 2026 15:42
Allows the sparsity pattern in PetscPreconditioner for SNES, CVODE and
PETSc interfaces to be set using a PetscOperator.

This should enable preconditioning of 3D FCI simulations.
Some reorganisation, removing duplicate and obsolete sections.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread include/bout/solver.hxx
#if BOUT_HAS_PETSC
struct DeferredJacobianPattern {
bout::petsc::UniqueMat submatrix{new Mat{nullptr}};
VarRef out_var{};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: initializer for member 'out_var' is redundant [readability-redundant-member-init]

Suggested change
VarRef out_var{};
VarRef out_var;

Comment thread include/bout/solver.hxx
struct DeferredJacobianPattern {
bout::petsc::UniqueMat submatrix{new Mat{nullptr}};
VarRef out_var{};
VarRef in_var{};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: initializer for member 'in_var' is redundant [readability-redundant-member-init]

Suggested change
VarRef in_var{};
VarRef in_var;

int run() override;
BoutReal run(BoutReal tout);
#if BOUT_HAS_PETSC
bool addJacobianPattern(const PetscCellOperator& op, VarRef out_var,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "PetscCellOperator" is directly included [misc-include-cleaner]

src/solver/impls/cvode/cvode.hxx:32:

- #include "bout/solver.hxx"
+ #include "bout/petsc_operators.hxx"
+ #include "bout/solver.hxx"


int init() override;
int run() override;
bool addJacobianPattern(const PetscCellOperator& op, VarRef out_var,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "PetscCellOperator" is directly included [misc-include-cleaner]

src/solver/impls/petsc/petsc.hxx:29:

- #include "bout/solver.hxx"
+ #include "bout/petsc_operators.hxx"
+ #include "bout/solver.hxx"


int init() override;
int run() override;
#if BOUT_HAS_PETSC

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: nested redundant #if; consider removing it [readability-redundant-preprocessor]

#if BOUT_HAS_PETSC
 ^
Additional context

src/solver/impls/snes/snes.hxx:33: previous #if was here

#if BOUT_HAS_PETSC
 ^

Comment thread src/solver/solver.cxx
}

#if BOUT_HAS_PETSC
bool Solver::addJacobianPattern(const PetscCellOperator& op) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "PetscCellOperator" is directly included [misc-include-cleaner]

src/solver/solver.cxx:39:

- #include "bout/region.hxx"
+ #include "bout/petsc_operators.hxx"
+ #include "bout/region.hxx"

Comment thread src/solver/solver.cxx
}

auto out_mapping =
std::dynamic_pointer_cast<const PetscCellMapping>(op.getOutMapping());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "PetscCellMapping" is directly included [misc-include-cleaner]

      std::dynamic_pointer_cast<const PetscCellMapping>(op.getOutMapping());
                                      ^

Comment thread src/solver/solver.cxx
}

const auto evolve_boundary = [](const auto& field) { return field.evolve_bndry; };
return std::none_of(f2d.begin(), f2d.end(), evolve_boundary)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "std::none_of" is directly included [misc-include-cleaner]

src/solver/solver.cxx:48:

+ #include <algorithm>

Comment thread src/solver/solver.cxx

const auto evolve_boundary = [](const auto& field) { return field.evolve_bndry; };
return std::none_of(f2d.begin(), f2d.end(), evolve_boundary)
&& std::none_of(f3d.begin(), f3d.end(), evolve_boundary);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "std::none_of" is directly included [misc-include-cleaner]

         && std::none_of(f3d.begin(), f3d.end(), evolve_boundary);
                 ^

Comment thread src/solver/solver.cxx
const int nvars = n2Dvars() + n3Dvars();
ASSERT1(nvars > 0);

BOUT_DO_PETSC(MatSetOption(Jfd, MAT_NEW_NONZERO_ALLOCATION_ERR, PETSC_FALSE));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "BOUT_DO_PETSC" is directly included [misc-include-cleaner]

src/solver/solver.cxx:39:

- #include "bout/region.hxx"
+ #include "bout/petsclib.hxx"
+ #include "bout/region.hxx"

@bendudson bendudson changed the title WIP: Calculate Jacobian coloring using PetscOperator matrices Calculate Jacobian coloring using PetscOperator matrices Jul 1, 2026
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.

1 participant