Extract TRACK Ordering Into Open-Path TSP Solver#5221
Conversation
bska
added
the
manual:irrelevant
|
While obviously still a heuristic, this is nevertheless a slightly more formal approach than proposed in PR #5208. I'm creating the PR in draft mode to allow comparison between this and #5208 and to gain experience on real problems. I will keep the PR in a draft state until we make a decision on which approach to use; i.e., PR #5208 or this strategy. |
There was a problem hiding this comment.
Pull request overview
This PR refactors well connection TRACK ordering by extracting the ordering logic out of WellConnections into a new TrackOrderingTSP heuristic solver, and adds dedicated unit tests for the new behavior.
Changes:
- Introduces
Opm::TrackOrderingTSP(multi-start nearest-neighbour + open-path 2-opt) to compute a TRACK ordering permutation. - Updates
WellConnections::orderTRACK()to delegate ordering toTrackOrderingTSPand apply the returned permutation. - Adds a focused unit test suite for the new solver and wires new sources/headers into CMake.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
tests/parser/TestTrackOrderingTSP.cpp |
New unit tests for TRACK ordering behavior, weights, and reproducibility. |
opm/input/eclipse/Schedule/Well/WellConnections.hpp |
Removes the old nearest-neighbour helper declaration now superseded by the new solver. |
opm/input/eclipse/Schedule/Well/WellConnections.cpp |
Replaces in-class TRACK ordering logic with a call to TrackOrderingTSP and reorders connections via returned indices. |
opm/input/eclipse/Schedule/Well/TrackOrderingTSP.hpp |
Declares the new TRACK ordering solver and public API (weights, multi-start configuration, ordering). |
opm/input/eclipse/Schedule/Well/TrackOrderingTSP.cpp |
Implements the TRACK edge cost, greedy path construction, and 2-opt improvement. |
CMakeLists_files.cmake |
Adds the new solver implementation and new test file to the build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
PR #5208 is just an attempt to show the way to get the correct solution for the single case that triggered the issue. It has never been intended to be a proper solution. I think we should focus on this PR as long as it produces the desired results. |
bska
force-pushed
the
track-order-tsp-opt2
branch
3 times, most recently
from
cca3e4a to
8ec0024
Compare
bska
force-pushed
the
track-order-tsp-opt2
branch
from
8240493 to
72d7257
Compare
bska
force-pushed
the
track-order-tsp-opt2
branch
2 times, most recently
from
3c3586b to
98a5eec
Compare
bska
force-pushed
the
track-order-tsp-opt2
branch
5 times, most recently
from
41b8f22 to
5d97be4
Compare
|
jenkins build this please |
bska
force-pushed
the
track-order-tsp-opt2
branch
from
381925f to
547bcf1
Compare
bska
force-pushed
the
track-order-tsp-opt2
branch
from
f714fde to
2c5d370
Compare
bska
force-pushed
the
track-order-tsp-opt2
branch
from
ca5694d to
8ccd064
Compare
bska
force-pushed
the
track-order-tsp-opt2
branch
from
b714ce9 to
e52ba12
Compare
bska
force-pushed
the
track-order-tsp-opt2
branch
2 times, most recently
from
c766c71 to
1ff9fc3
Compare
bska
force-pushed
the
track-order-tsp-opt2
branch
from
db3267f to
5a307ee
Compare
bska
force-pushed
the
track-order-tsp-opt2
branch
from
5a307ee to
4bb5640
Compare
atgeirr
left a comment
atgeirr
left a comment
There was a problem hiding this comment.
I enjoyed reading this, and have found no real bugs, good work! My comments mostly relate to improving variable names and comments, with the exception of the "number of start candidates to return" issue, which I think is worth considering again.
The larger question is how much this will change existing behaviour. We should probably look at some extra field cases to evaluate that. In the worst case, we need to keep supporting the existing behaviour (with the #5208 changes) in addition to this, but I hope not.
| struct TrackCostWeights | ||
| { | ||
| /// Weight for horizontal logical-grid distance in the I/J plane. | ||
| double w_xy {1.0}; |
There was a problem hiding this comment.
Suggest w_ij instead to be consistent with the w_k name, and that these are IJ not xy distances weighted.
| /// connections and their step vector. | ||
| double w_dir {0.25}; | ||
|
|
||
| /// Scale factor that maps depth differences into the I/J distance |
There was a problem hiding this comment.
I do not really understand what this means, consider adding a bit more explanation.
|
|
||
| auto mid = start_candidates.begin() + num_starts; | ||
| if (mid != start_candidates.end()) { | ||
| std::ranges::nth_element(start_candidates, mid); |
There was a problem hiding this comment.
This functions always returns a vector of candidates equal to the number of connections. Why not just return the top numStartPoints_ candidates? If we choose to return the full number of connections, I think it would be prudent to sort all of them, not just the top numStartPoints_ ones.
Either way, the statement in the doc for this function:
"Start point candidates from index \p numStartPoints_ and up are unordered and should typically not be referenced." could then be removed (for one reason or the other).
| { | ||
| constexpr auto max_cost = std::numeric_limits<std::int_least64_t>::max(); | ||
|
|
||
| const auto dx = static_cast<double>(b.getI() - a.getI()); |
There was a problem hiding this comment.
Suggest di, dj instead of dx, dy. Similar with abs_dx etc.
| return max_cost; | ||
| } | ||
|
|
||
| // The multiplicative factor 1.0e9 effectively imposes a 1.0e-9 |
There was a problem hiding this comment.
That comment I do not understand, also it seems that improvement thresholds should be up to the optimization algo, not the distance metric to decide?
| /// \param[in] w Cost weights controlling the distance terms. | ||
| /// | ||
| /// \return Weighted cost of moving from \p a to \p b. | ||
| std::int_least64_t |
There was a problem hiding this comment.
Why use an integer type for this?
| const auto dk = static_cast<double>(b.getK() - a.getK()); | ||
| const auto dz = std::abs(b.depth() - a.depth()); | ||
|
|
||
| const auto abs_dx = std::abs(dx); |
There was a problem hiding this comment.
Suggest including the std::abs in all the d... variables, not just dz.
| /// dominant geometric direction of the completion. | ||
| /// | ||
| /// \param[in] dir Completion direction being evaluated. | ||
| /// \param[in] abs_dx Absolute logical I-step length. |
There was a problem hiding this comment.
Suggest abs_di etc. as elsewhere.
| /// Primary sort key. | ||
| std::int_least64_t ijdist2 {}; | ||
|
|
||
| /// Connection depth difference relative to reference depth. |
There was a problem hiding this comment.
I first misunderstood this to be a relative difference, but I see from the implementation elsewhere that the intention is for this to be essentially abs(depth - surface_depth), with surface_depth hardcoded to zero. Suggest rephrasing slightly, removing the word relative that confused me in the first place.
bska
force-pushed
the
track-order-tsp-opt2
branch
from
4bb5640 to
0140242
Compare
Move TRACK completion ordering out of WellConnections and into a new
TrackOrderingTSP class.
The key change is to treat TRACK ordering as an open (path)
travelling salesman problem over connections, rather than as a pure
nearest-neighbour search inside WellConnections. The solver returns
a permutation of input indices, and WellConnections::orderTRACK() is
now only a thin adapter that applies that permutation.
Heuristic choices and optimisation strategy:
* Define a weighted edge cost over (I, J, K, depth, direction):
w_xy * hypot(dI, dJ) +
w_k * |dK| +
w_z * |dZ| +
w_dir * direction_penalty.
* Keep existing heel semantics by picking the heel as the
completion closest to (headI, headJ), tie-broken by shallowest
depth.
* Use multi-start nearest-neighbour construction:
user configurable number of starting point candidates, though
always at least one--the well heel--sorted by IJ distance,
then depth.
* Improve each candidate path with open-path 2-opt; reverse only
suffix segments when a strict cost reduction is found.
* Select the minimum-cost path over all starts to get
deterministic, reproducible ordering.
Also add focused unit coverage in TestTrackOrderingTSP for
empty/single inputs, heel selection, vertical/horizontal/L-shaped
paths, direction penalty behavior, 2-opt de-crossing,
reproducibility, and custom weights.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
bska
force-pushed
the
track-order-tsp-opt2
branch
from
0140242 to
55d3a19
Compare
Move TRACK completion ordering out of
WellConnectionsand into a newTrackOrderingTSPclass.The key change is to treat TRACK ordering as an open (path) travelling salesman problem over connections, rather than as a pure nearest-neighbour search inside
WellConnections. The solver returns a permutation of input indices, andWellConnections::orderTRACK()is now only a thin adapter that applies that permutation.Heuristic choices and optimisation strategy:
Define a weighted edge cost over (I, J, K, depth, direction):
w_xy * hypot(dI, dJ) +
w_k * |dK| +
w_z * |dZ| +
w_dir * direction_penalty.
Keep existing heel semantics by picking the heel as the completion closest to (headI, headJ), tie-broken by shallowest depth.
Use multi-start nearest-neighbour construction: user configurable number of starting point candidates, though always at least one--the well heel--sorted by IJ distance, then depth.
Improve each candidate path with open-path 2-opt; reverse only suffix segments when a strict cost reduction is found.
Select the minimum-cost path over all starts to get deterministic, reproducible ordering.
Also add focused unit coverage in TestTrackOrderingTSP for empty/single inputs, heel selection, vertical/horizontal/L-shaped paths, direction penalty behavior, 2-opt de-crossing, reproducibility, and custom weights.