Skip to content

Remove the unused NOX include and C++ standard flag for Trilinos builds - #716

Merged
zasexton merged 3 commits into
SimVascular:mainfrom
zasexton:issue-709
Oct 29, 2026
Merged

zasexton merged 3 commits into
SimVascular:mainfrom
zasexton:issue-709

Conversation

@zasexton

Copy link
Copy Markdown
Collaborator

Current situation

trilinos_impl.h includes NOX_TpetraTypedefs.hpp but uses nothing from NOX, so the solver cannot be built against a Trilinos installation without NOX. Trilinos builds also append -std=c++${CMAKE_CXX_STANDARD}, which has no effect because CMake adds its own standard flag after it. This PR removes the include and the flag.

Fixes #709.

Release Notes

  • The solver builds against Trilinos installations without NOX.

Documentation

No documentation changes are needed.

Testing

  • Built the solver against Trilinos 16.2.2 installed without NOX, where the removed include fails to compile, and passed the unit tests.
  • Passed the Trilinos tests with the ILUT, block Jacobi, RILUK, and MueLu preconditioners (7 tests with 1, 3, and 4 processes).

Code of Conduct & Contributing Guidelines

trilinos_impl.h uses nothing from NOX, so the include only prevented builds
against Trilinos without NOX. The appended -std flag had no effect because
CMake adds the standard flag after CMAKE_CXX_FLAGS.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@codecov

codecov Bot commented Oct 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.45%. Comparing base (8df7565) to head (845fd81).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #716      +/-   ##
==========================================
+ Coverage   73.43%   73.45%   +0.01%     
==========================================
  Files         281      281              
  Lines       40720    40768      +48     
  Branches     6762     6770       +8     
==========================================
+ Hits        29903    29945      +42     
- Misses      10561    10567       +6     
  Partials      256      256              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sujaldave sujaldave left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Apologies for taking time on this.

To be sure I wasn't using NOX even during runtime (somehow on Kokkos or TPetra) on CPUs and GPUs - I built the branch on Sherlock CPUs and GPUs and ran simple fluid/fsi pipe3D tests as well. All successful. Agree with the two minor removals.

@bearsontheloose bearsontheloose left a comment

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.

This looks good.

@zasexton
zasexton merged commit b9c2ddc into SimVascular:main Oct 29, 2026
11 checks passed

This branch was previously deployed

1 inactive deployment
review-policy — 234a7f8d Deployed Oct 29, 2026 by zasexton via evaluate #1700
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.

Remove the unused NOX include and C++ standard flag for Trilinos builds

4 participants