Skip to content

Fixing warning #1044

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
daavid00 wants to merge 1 commit into
base: master
Choose a base branch
from
+8 −10
Open

Fixing warning #1044

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 8 additions & 10 deletions opm/grid/common/ZoltanGraphFunctions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,17 +183,15 @@ template <typename EdgeWeightType>
EdgeWeightType sumOfGridEdges(const Dune::CpGrid& grid,
const CombinedGridWellGraph& graph)
{
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);

@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.

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.

@michal-toth michal-toth 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.

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 •
edited
Loading

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.

if (weWeight >= std::numeric_limits<EdgeWeightType>::max()-addend) {
weWeight = std::numeric_limits<EdgeWeightType>::max();
break;
}
weWeight += graph.transmissibility(edge);
double total = 0.0;
for (int edge = 0; edge < grid.numFaces(); ++edge) {
total += graph.transmissibility(edge);
}
const double maxVal = static_cast<double>(std::numeric_limits<EdgeWeightType>::max());
if (total > maxVal) {
return std::numeric_limits<EdgeWeightType>::max();
}
return weWeight;
return static_cast<EdgeWeightType>(total);
}

template<typename ID>
Expand Down