Skip to content

Fixing warning#1044

Open
daavid00 wants to merge 1 commit into
OPM:masterfrom
daavid00:warning
Open

Fixing warning#1044
daavid00 wants to merge 1 commit into
OPM:masterfrom
daavid00:warning

Conversation

@daavid00

Copy link
Copy Markdown
Member
opm-grid/opm/grid/common/ZoltanGraphFunctions.cpp:190:25: warning: implicit conversion from 'type' (aka 'long') to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Wimplicit-const-int-float-conversion]
  190 |         if (weWeight >= std::numeric_limits<EdgeWeightType>::max()-addend) {

@daavid00 daavid00 added the manual:irrelevant This PR is a minor fix and should not appear in the manual label Jun 22, 2026
@daavid00

Copy link
Copy Markdown
Member Author

jenkins build this please

EdgeWeightType weWeight = 0;
for (int edge=0; edge<grid.numFaces(); ++edge) {
// assumes that transmissibility is positive (too obvious to leave an assert)
const auto& addend = graph.transmissibility(edge);

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 am not sure how big these transmissibilities can get, but from the point of view of what is computed here, I think is better to do the whole computation on the original type which is always double and cast to the target type when using the value.

That has several advantages:

  • Avoid explicit template instantiations for each used type which is more maintainable. This will avoid linker error if someone needs to cast to another type later on.
  • On values that are very small for each edge, double will have a better truncation than the currently used types which are float and int. Small values (e.g., 0.4 for integers) truncate to zero prematurely, which I'd imagine is not desirable.
  • The current implementation is this way because adding on a saturated integer wraps around (e.g., the correct form even made into the standard), which is would lead to errors. With a floating point type, saturation is automatic.

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.

The transmissibilities should not get big enough to reach the limit of float or int. In CombinedGridWellGraph the transmissibility is multiplied by 1e18 because of metis which operates on integers and values are shifted from decimals. Thus metis is at higher risk of reaching the limit but I would be very surprised if it happened.

@SoilRos SoilRos Jun 26, 2026

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.

@michal-toth I see, the scaling makes sense for metis.

TBH, this looks a bit confusing to me. How I understand it, the scaled value is the edge weight, not a transmissibility. However, there is another method that computes the edgeWeight. Should't the transmissibility simply return whatever the transmissibility vector contains, and let the edgeWeight perform the actual scaling? Or is there an specific reason why not to use edgeWeight in this function? At least, that's what I would expect when reading this function documentation.

@SoilRos SoilRos requested a review from Copilot June 26, 2026 12:31

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@daavid00

Copy link
Copy Markdown
Member Author

jenkins build this please

@daavid00

Copy link
Copy Markdown
Member Author

jenkins build this please

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