Skip to content

Added stand alone test for primary variables. #7046

Open
hnil wants to merge 2 commits into
OPM:masterfrom
hnil:test_primary_variables
Open

Added stand alone test for primary variables. #7046
hnil wants to merge 2 commits into
OPM:masterfrom
hnil:test_primary_variables

Conversation

@hnil
Copy link
Copy Markdown
Member

@hnil hnil commented May 12, 2026

Added standalone test for primaryvariable switching. It will be extended later.

@hnil hnil requested review from GitPaean, atgeirr and bska May 12, 2026 11:01
@hnil hnil added the manual:irrelevant This PR is a minor fix and should not appear in the manual label May 12, 2026
@GitPaean
Copy link
Copy Markdown
Member

jenkins build this please

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a new Boost.Test unit test module to exercise Opm::BlackOilPrimaryVariables::adaptPrimaryVariables() switching logic (Sw/Sg/Rs/Rv) using a stub Problem and a zero-capillary material law, and wires the new test into the CMake test sources list.

Changes:

  • Introduces tests/test_blackoilprimaryvariables.cpp with several focused test cases for primary-variable meaning switching.
  • Adds the new test file to TEST_SOURCE_FILES in CMakeLists_files.cmake.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File Description
tests/test_blackoilprimaryvariables.cpp New standalone unit tests for adaptPrimaryVariables() switching between Sg/Rs/Rv and pressure meaning updates under controlled FluidSystem settings.
CMakeLists_files.cmake Registers the new test source so it is built and run with the existing unit test suite.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +11 to +13
#include <stdexcept>
#include <tuple>
#include <vector>
Comment on lines +205 to +210
BOOST_CHECK(changed);
BOOST_CHECK(priVars.primaryVarsMeaningGas() == PrimaryVariables::GasMeaning::Sg);
BOOST_CHECK(priVars.primaryVarsMeaningWater() == PrimaryVariables::WaterMeaning::Sw);
BOOST_CHECK_CLOSE(priVars[Indices::waterSwitchIdx], 1.0, 1e-12);
BOOST_CHECK_SMALL(priVars[Indices::compositionSwitchIdx], 1e-12);
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think these should be fine since there is no calculation of them. But you can decide.

BOOST_CHECK(changed);
BOOST_CHECK(priVars.primaryVarsMeaningGas() == PrimaryVariables::GasMeaning::Rs);
BOOST_CHECK(priVars.primaryVarsMeaningPressure() == PrimaryVariables::PressureMeaning::Po);
BOOST_CHECK_CLOSE(priVars[Indices::compositionSwitchIdx], 0.25, 1e-12);
Comment on lines +303 to +308
BOOST_CHECK(changed);
BOOST_CHECK(priVars.primaryVarsMeaningGas() == PrimaryVariables::GasMeaning::Rv);
BOOST_CHECK(priVars.primaryVarsMeaningPressure() == PrimaryVariables::PressureMeaning::Pg);
BOOST_CHECK_CLOSE(priVars[Indices::compositionSwitchIdx], 0.15, 1e-12);
BOOST_CHECK_CLOSE(priVars[Indices::pressureSwitchIdx], 1.5e5, 1e-12);
}
Comment on lines +333 to +337
BOOST_CHECK(changed);
BOOST_CHECK(priVars.primaryVarsMeaningGas() == PrimaryVariables::GasMeaning::Sg);
BOOST_CHECK(priVars.primaryVarsMeaningPressure() == PrimaryVariables::PressureMeaning::Po);
BOOST_CHECK_CLOSE(priVars[Indices::compositionSwitchIdx], 0.8, 1e-12);
BOOST_CHECK_CLOSE(priVars[Indices::pressureSwitchIdx], 1.5e5, 1e-12);
BOOST_CHECK(priVars.primaryVarsMeaningPressure() == PrimaryVariables::PressureMeaning::Po);
BOOST_CHECK_CLOSE(priVars[Indices::compositionSwitchIdx], 0.8, 1e-12);
BOOST_CHECK_CLOSE(priVars[Indices::pressureSwitchIdx], 1.5e5, 1e-12);
} No newline at end of file
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

missing an end-of-file, something like that.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

, gasPvt(FluidSystem::gasPvt())
, oilPvt(FluidSystem::oilPvt())
{
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

not important, while since you defined a non-default constructor. suggested by the ide.

+    FluidSystemGuard(const FluidSystemGuard&) = delete;
+    FluidSystemGuard& operator=(const FluidSystemGuard&) = delete;
+    FluidSystemGuard(FluidSystemGuard&&) = delete;
+    FluidSystemGuard& operator=(FluidSystemGuard&&) = delete;

Copy link
Copy Markdown
Member

@GitPaean GitPaean left a comment

Choose a reason for hiding this comment

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

I went through the tests, it looks sensible and appears to test what the function adaptPrimaryVariables() is supposed to do.

The PR can go in after the few small comments are addressed.

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

Labels

manual:irrelevant This PR is a minor fix and should not appear in the manual

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants