Skip to content

converting std::vector<double> to std::vector<float> for BIC coefficients in GenericOilGasWaterFluidSystem#5178

Merged
bska merged 3 commits into
OPM:masterfrom
GitPaean:fixing_vector_float_double
Jun 3, 2026
Merged

converting std::vector<double> to std::vector<float> for BIC coefficients in GenericOilGasWaterFluidSystem#5178
bska merged 3 commits into
OPM:masterfrom
GitPaean:fixing_vector_float_double

Conversation

@GitPaean
Copy link
Copy Markdown
Member

for testing the PR OPM/opm-simulators#7049 .

@GitPaean GitPaean added the manual:irrelevant This PR is a minor fix and should not appear in the manual label May 21, 2026
@GitPaean GitPaean marked this pull request as ready for review May 27, 2026 07:25
@GitPaean GitPaean force-pushed the fixing_vector_float_double branch from ec4a8bc to 1730d97 Compare May 27, 2026 07:29
@GitPaean GitPaean requested a review from Copilot May 27, 2026 07:29
@GitPaean
Copy link
Copy Markdown
Member Author

jenkins build this please

@GitPaean
Copy link
Copy Markdown
Member Author

It is used to compile the float version of flow variants.

https://ci.opm-project.org/job/opm-simulators-PR-builder/9985/console

But it is also possible the refactoring in OPM/opm-simulators#7049 should not have triggered this issue.

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

Updates GenericOilGasWaterFluidSystem initialization to allow BIC (binary interaction coefficient) data coming from CompositionalConfig (std::vector<double>) to be stored in the fluid system’s interaction_coefficients_ (std::vector<Scalar>), enabling builds where Scalar is float (as needed for the referenced simulator PR).

Changes:

  • Replace direct assignment of BIC (std::vector<double>) into interaction_coefficients_ with an iterator-based copy to support differing element types (doubleScalar).
  • Introduce a local bic reference for the retrieved BIC vector.

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

Comment thread opm/material/fluidsystems/GenericOilGasWaterFluidSystem.hpp Outdated
Comment thread opm/material/fluidsystems/GenericOilGasWaterFluidSystem.hpp Outdated
@GitPaean GitPaean marked this pull request as draft May 27, 2026 07:36
@GitPaean
Copy link
Copy Markdown
Member Author

It is used to compile the float version of flow variants.

https://ci.opm-project.org/job/opm-simulators-PR-builder/9985/console

But it is also possible the refactoring in OPM/opm-simulators#7049 should not have triggered this issue.

Without looking into details yet, since OPM/opm-simulators#7049 is trying to make compositial based on the Flow framework, when the instantiation based on the GenericOilGasWaterFluidSystem for FlowGenericProblem might be necessary.

@GitPaean
Copy link
Copy Markdown
Member Author

jenkins build this please

@GitPaean GitPaean force-pushed the fixing_vector_float_double branch from e73a736 to 2490261 Compare June 3, 2026 10:21
@GitPaean
Copy link
Copy Markdown
Member Author

GitPaean commented Jun 3, 2026

It is needed if (-DBUILD_FLOW_FLOAT_VARIANTS is on) through OPM/opm-simulators#7049 .

@GitPaean GitPaean marked this pull request as ready for review June 3, 2026 10:22
@GitPaean
Copy link
Copy Markdown
Member Author

GitPaean commented Jun 3, 2026

should we do it through different way? @bska @akva2

@hnil hnil requested a review from totto82 June 3, 2026 12:02
Copy link
Copy Markdown
Member

@akva2 akva2 left a comment

Choose a reason for hiding this comment

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

Looks fine to me

@GitPaean
Copy link
Copy Markdown
Member Author

GitPaean commented Jun 3, 2026

jenkins build this please

@akva2
Copy link
Copy Markdown
Member

akva2 commented Jun 3, 2026

though clang might not agree, so maybe something akin to https://github.com/OPM/opm-simulators/blob/master/opm/simulators/flow/equil/InitStateEquil_impl.hpp#L1528 is necessary to shut it up

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.

Can we at least keep the current assignment if Scalar is double? For instance, something along the lines of

if constexpr (std::is_same_v<Scalar, double>) {
    this->interaction_coefficients_ = ...;
}
else {
    const auto& bic = ...;
    this->interaction_coefficients_.assign(bic.begin(), bic.end());
}

with std::is_same_v<> from the <type_traits> header.

@GitPaean GitPaean force-pushed the fixing_vector_float_double branch from 2490261 to 2ffc30f Compare June 3, 2026 13:31
@GitPaean
Copy link
Copy Markdown
Member Author

GitPaean commented Jun 3, 2026

I combined both comments, please let me know if it is okay.

Implicit conversion from double to float will always be there with either approach?

@GitPaean
Copy link
Copy Markdown
Member Author

GitPaean commented Jun 3, 2026

jenkins build this please

Copy link
Copy Markdown
Member

@bska bska left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the updates. This looks good to me now and I'll merge into master.

@bska bska merged commit 59fc570 into OPM:master Jun 3, 2026
2 checks passed
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