Skip to content

Test diagonal and upper triangular matrix coefficients in pressureVelocityCoupling test#275

Merged
greole merged 4 commits into
developfrom
fix/failingDiagTest
May 13, 2026
Merged

Test diagonal and upper triangular matrix coefficients in pressureVelocityCoupling test#275
greole merged 4 commits into
developfrom
fix/failingDiagTest

Conversation

@greole
Copy link
Copy Markdown
Contributor

@greole greole commented Apr 26, 2026

This PR is the counterpart to exasim-project/NeoN#495 and adds a test to the pressureVelocityCoupling tests to ensure the correctness of the diagonal and upper triangular matrix coefficients.

Additionally:

  • fix order of arguments for the explicit grad benchmark

@github-actions
Copy link
Copy Markdown

Deployed test documentation to https://exasim-project.com/NeoFOAM/Build_PR_275

Comment thread test/test_pressureVelocityCoupling.cpp Outdated
ofUEqn.upper(),
ApproxVector(1e-15)
);
nf::compare(nfUEqn.linearSystem().matrix().diag(), ofUEqn.diag(), ApproxVector(1e-15));
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@HenningScheufler, @HendriceH any idea why this test fails?

Copy link
Copy Markdown
Contributor Author

@greole greole Apr 26, 2026

Choose a reason for hiding this comment

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

I guess this is what 374a198 fixes.

Copy link
Copy Markdown
Contributor Author

@greole greole Apr 26, 2026

Choose a reason for hiding this comment

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

potentially related PRs 436 and 204

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, I guess I was stupid and we discussed this several times already, but just for reference: we include the boundary contributions to the diagonal elements in the Laplacian and div kernel already (Kokkos::atomic_sub(&values[rowOwnStart + diagOffs[own]], valueMat); ), but OpenFOAM doesn't.

@greole greole added stack and removed stack labels May 5, 2026
@greole greole force-pushed the fix/failingDiagTest branch from 9eb32e8 to df62c18 Compare May 12, 2026 06:32
@greole greole changed the title Fix/failing diag test Test diagonal and upper triangular matrix coeffecients in pressureVelocityCoupling test May 12, 2026
@greole greole changed the title Test diagonal and upper triangular matrix coeffecients in pressureVelocityCoupling test Test diagonal and upper triangular matrix coefficients in pressureVelocityCoupling test May 12, 2026
@greole greole force-pushed the fix/failingDiagTest branch from a185016 to 7ec5228 Compare May 12, 2026 11:07
Copy link
Copy Markdown
Collaborator

@chihtaw chihtaw left a comment

Choose a reason for hiding this comment

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

LGTM!
The only two things that I recommend before merge is adding explanatory comments around removeBoundaryContributions usage in the test and considering a tolerance which is not too strict for GPU backends.

Comment thread test/test_pressureVelocityCoupling.cpp
nf::compare(
NeoN::la::upper(nfUEqn.linearSystem().matrix()),
ofUEqn.upper(),
ApproxVector(1e-15)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The current tolerance may be overly strict on GPU backends.
This is probably fine on CPU/deterministic runs, but may become fragile on GPU. Consider something closer to 1e-12.

Co-authored-by: Chih-Ta Wang <97195768+chihtaw@users.noreply.github.com>
@greole greole force-pushed the fix/failingDiagTest branch from 386136c to 42d2063 Compare May 13, 2026 12:53
@greole greole merged commit e7f2399 into develop May 13, 2026
7 of 9 checks passed
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.

2 participants