Skip to content

[WIP] Improve integrals of scalar functions for postprocessing#273

Open
sebastiangrimberg wants to merge 11 commits intomainfrom
sjg/integrate-function-dev
Open

[WIP] Improve integrals of scalar functions for postprocessing#273
sebastiangrimberg wants to merge 11 commits intomainfrom
sjg/integrate-function-dev

Conversation

@sebastiangrimberg
Copy link
Contributor

@sebastiangrimberg sebastiangrimberg commented Jul 11, 2024

WIP

  • Thread-safety of GridFunction evaluation and face/boundary coefficients in coefficient.cpp
  • Address GPU usage blocker

@sebastiangrimberg sebastiangrimberg added performance Related to performance postprocessing Related to simulation postprocessing labels Jul 11, 2024
@sebastiangrimberg sebastiangrimberg marked this pull request as draft July 11, 2024 16:11
@sebastiangrimberg sebastiangrimberg force-pushed the sjg/integrate-function-dev branch from 42dfbdd to 0d3003b Compare July 11, 2024 20:09
@hughcars hughcars force-pushed the sjg/integrate-function-dev branch from 0d3003b to 983c34b Compare June 3, 2025 18:06
@hughcars hughcars force-pushed the sjg/integrate-function-dev branch 5 times, most recently from cc509e7 to c1a37ca Compare February 7, 2026 00:54
sebastiangrimberg and others added 11 commits February 9, 2026 19:21
Remove shared FET, T1, T2 member variables from BdrGridFunctionCoefficient
base class. The base instead stores a vector of instances, similar to `ceeds`.
Introduces `using ConstantCoefficient = mfem::ConstantCoefficient` in the
palace namespace to seal the abstraction. Updates usages in laplaceoperator
and geodata. Adds unit tests for InnerProductCoefficient and
BdrInnerProductCoefficient.
The Eval methods being overridden are not const, so the helper methods
don't need to be const either. This avoids hiding mutation effects.
Replace manual double[3] buffer + mfem::Vector pattern with StaticVector<3>
for cleaner, type-safe stack allocation in coefficient evaluation methods.
@hughcars hughcars force-pushed the sjg/integrate-function-dev branch from c1a37ca to 9cc6a54 Compare February 10, 2026 00:21
@hughcars hughcars marked this pull request as ready for review February 10, 2026 00:21
Copy link
Member

@Sbozzolo Sbozzolo left a comment

Choose a reason for hiding this comment

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

I started the review and I think that this PR introduces an important regression for GPUs.

My understanding is that code like the following

    mfem::LinearForm pr(&nd_fespace);
    pr.AddBoundaryIntegrator(new VectorFEBoundaryLFIntegrator(nxHr_func), attr_marker);
    pr.UseFastAssembly(false);
    pr.UseDevice(false);
    pr.Assemble();
    pr.UseDevice(true);
    dot = -(pr * E.Real()) - 1i * (pr * E.Imag());

performs the integration on the device.

The new implementation no longer relies on this approach and performs everything on the CPU.

Unfortunately, I don't think there's an easy way to checking for this type of problems programatically. The only way I know to confirm that a given piece of code is running on GPU is to use Nsight (combined with an NVTX range). Correctness alone cannot be relied upon because the code could still be correct but not running on GPU. Memory transfers would also not be reliable because memory could be moved to the device, but the algorithm could still be running on CPU.

: mesh(mesh), scaling(scaling)
{
// Only use multiple entries when not on GPU (matches libCEED initialization).
int nt = mfem::Device::Allows(mfem::Backend::DEVICE_MASK) ? 1 : utils::GetMaxThreads();
Copy link
Member

Choose a reason for hiding this comment

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

We have a device.cpp in utils. Should we just add a function to return whether we are on GPU or not (not sure how that would look like), instead of this obscure mfem::Device::Allows(mfem::Backend::DEVICE_MASK) syntax?

Comment on lines +37 to +43
double IntegrateFunction(
const mfem::ParMesh &mesh, const mfem::Array<int> &marker, bool bdr,
mfem::Coefficient &Q,
std::function<int(const mfem::ElementTransformation &)> GetQuadratureOrder =
[](const mfem::ElementTransformation &T)
{ return DefaultIntegrationOrder::Get(T); });
double IntegrateFunctionLocal(
Copy link
Member

Choose a reason for hiding this comment

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

Can you add docstrings to these functions?


public:
VectorFEBoundaryLFIntegrator(mfem::VectorCoefficient &QG) : Q(QG) {}
VectorFEBoundaryLFIntegrator(mfem::VectorCoefficient &Q) : Q(Q) {}
Copy link
Member

Choose a reason for hiding this comment

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

Is this used?


public:
BoundaryLFIntegrator(mfem::Coefficient &QG) : Q(QG) {}
BoundaryLFIntegrator(mfem::Coefficient &Q) : Q(Q) {}
Copy link
Member

Choose a reason for hiding this comment

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

Is this used?

@Sbozzolo
Copy link
Member

See also #252

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance Related to performance postprocessing Related to simulation postprocessing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants