好的,准备搭车。
- 首先我发现了一个错误
- 接下来,我全面审查、重构和简化了代码
- 当一切尘埃落定后,我注意到一个行为变化,看起来像是代码中的潜在逻辑错误
1。错误
就像我在问题中评论的那样,由于过度依赖没有明确语义的原始指针,代码复杂度很高。
在审查和重构代码时,我发现这确实导致了一个错误:
e->vertex->fib = new HeapData(e->vertex);
e->vertex->fib->handle = heap.push(*(e->vertex->fib));
- 在第一行创建一个 HeapData 对象。你让
fib 成员指向那个对象。
- 第二行插入该对象的副本(意思是,它是一个新对象,具有不同的对象标识,或者实际上是:不同的地址)。
那么,现在
-
e->vertex->fib 指向队列中不存在的(泄露的)HeapData 对象,并且
- 实际排队的
HeapData 副本有一个默认构造的handle 成员,这意味着句柄包装了一个空指针。 (检查 detail/stable_heap.hpp 中的 boost::heap::detail::node_handle<> 以验证这一点)。
这可以很好地解释您所看到的症状。
2。重构,简化
所以,在理解了代码之后,我得出的结论是
-
HeapData 和 Vertex 应该合并:HeapData 仅用于将句柄链接到顶点,但您已经可以让顶点直接包含句柄。
作为这次合并的结果
-
您的顶点队列现在实际上包含顶点,表达了代码的意图
-
您将所有顶点访问减少一级间接(减少 Law Of Demeter 违规)
-
您可以将推送操作写在一条自然的行中,从而消除您的错误出现的空间。之前:
target->fib = new HeapData(target);
target->fib->handle = heap.push(*(target->fib));
之后:
target->fibhandle = heap.push(target);
-
您的Edge 类实际上并不是对边缘进行建模,而是对“邻接”- 目标进行建模
边缘的一部分,带有权重属性。
为了清楚起见,我将其重命名为 OutEdge,并将向量更改为包含 values 而不是
动态分配OutEdge 实例。
我无法从显示的代码中看出,但我几乎可以保证这些是
被泄露了。
另外,OutEdge 在大多数平台上只有 16 个字节,因此复制它们就可以了,并且邻接根据定义归其源顶点所有(因为将其包含/移动到另一个源顶点会改变邻接的含义)。
In fact, if you're serious about performance, you may want to use a boost::container::small_vector with a suitably chosen capacity if you know that e.g.边数的中位数是“小”
-
您的比较可以“外包”给函数对象
using Node = Vertex*;
struct PrioCompare {
bool operator()(Node a, Node b) const;
};
在此之后堆可以键入为:
namespace bh = boost::heap;
using Heap = bh::fibonacci_heap<Node, bh::compare<PrioCompare>>;
using Handle = Heap::handle_type;
-
您的成本函数违反了更多的法则,这可以通过添加 Literate-Code 访问器轻松解决:
Cost cost() const { return distance + heuristic; }
-
通过快速检查,我认为使用infinite() 而非max() 作为初始距离会更准确。此外,为了便于阅读,请使用常量:
static constexpr auto INF = std::numeric_limits<Cost>::infinity();
Cost distance = INF;
-
您重复检查了xyz->equals(endPoint),以避免更新顶点的启发式。我建议将更新移动到顶点出列之后,这样就可以消除重复(检查和getHeuristic(...) 调用)。
-
就像你说的,我们需要小心处理increase/update 修复方法。当我阅读您的代码时,节点的优先级与“成本”(累积边权重和启发式值)成反比。
因为 Boost Heap 堆是 max heaps,这意味着增加优先级应该匹配降低成本。我们可以断言它来检测调试版本中的任何程序员错误:
assert(target->cost() < previous_cost);
heap.increase(target->fibhandle);
有了这些更改,代码可以更安静地阅读:
Cost AStarSearch(Node start, Node destination) {
Heap heap;
start->distance = 0;
start->fibhandle = heap.push(start);
while (!heap.empty()) {
Node u = heap.top();
heap.pop();
if (u->equals(destination)) {
return u->cost();
}
u->heuristic = getHeuristic(start);
doSomeGraphCreationStuff(u);
for (auto& [target, weight] : u->adj) {
auto curDistance = weight + u->distance;
// if cheaper route, queue or update queued
if (curDistance < target->distance) {
auto cost_prior = target->cost();
target->distance = curDistance;
target->predecessor = u;
if (target->fibhandle == NOHANDLE) {
target->fibhandle = heap.push(target);
} else {
assert(target->cost() < cost_prior);
heap.update(target->fibhandle);
}
}
}
}
return INF;
}
2(b) 现场演示
添加一些测试数据:
Live On Coliru
#include <boost/heap/fibonacci_heap.hpp>
#include <iostream>
using Cost = double;
struct Vertex;
Cost getHeuristic(Vertex const*) { return 0; }
void doSomeGraphCreationStuff(Vertex const*) {
// this only creates vertices and edges
}
struct OutEdge { // adjacency from implied source vertex
Vertex* target = nullptr;
Cost weight = 1;
};
namespace bh = boost::heap;
using Node = Vertex*;
struct PrioCompare {
bool operator()(Node a, Node b) const;
};
using Heap = bh::fibonacci_heap<Node, bh::compare<PrioCompare>>;
using Handle = Heap::handle_type;
static const Handle NOHANDLE{}; // for expressive comparisons
static constexpr auto INF = std::numeric_limits<Cost>::infinity();
struct Vertex {
Vertex(Cost d = INF, Cost h = 0) : distance(d), heuristic(h) {}
Cost distance = INF;
Cost heuristic = 0;
Handle fibhandle{};
Vertex* predecessor = nullptr;
std::vector<OutEdge> adj;
Cost cost() const { return distance + heuristic; }
Cost euclideanDistanceTo(Vertex* v);
bool equals(Vertex const* u) const { return this == u; }
};
// Now Vertex is a complete type, implement comparison
bool PrioCompare::operator()(Node a, Node b) const {
return a->cost() > b->cost();
}
Cost AStarSearch(Node start, Node destination) {
Heap heap;
start->distance = 0;
start->fibhandle = heap.push(start);
while (!heap.empty()) {
Node u = heap.top();
heap.pop();
if (u->equals(destination)) {
return u->cost();
}
u->heuristic = getHeuristic(start);
doSomeGraphCreationStuff(u);
for (auto& [target, weight] : u->adj) {
auto curDistance = weight + u->distance;
// if cheaper route, queue or update queued
if (curDistance < target->distance) {
auto cost_prior = target->cost();
target->distance = curDistance;
target->predecessor = u;
if (target->fibhandle == NOHANDLE) {
target->fibhandle = heap.push(target);
} else {
assert(target->cost() < cost_prior);
heap.update(target->fibhandle);
}
}
}
}
return INF;
}
int main() {
// a very very simple graph data structure with minimal helpers:
std::vector<Vertex> graph(10);
auto node = [&graph](int id) { return &graph.at(id); };
auto id = [&graph](Vertex const* node) { return node - graph.data(); };
// defining 6 edges
graph[0].adj = {{node(2), 1.5}, {node(3), 15}};
graph[2].adj = {{node(4), 2.5}, {node(1), 5}};
graph[1].adj = {{node(7), 0.5}};
graph[7].adj = {{node(3), 0.5}};
// do a search
Node startPoint = node(0);
Node endPoint = node(7);
Cost cost = AStarSearch(startPoint, endPoint);
std::cout << "Overall cost: " << cost << ", reverse path: \n";
for (Node node = endPoint; node != nullptr; node = node->predecessor) {
std::cout << " - " << id(node) << " distance " << node->distance
<< "\n";
}
}
打印
Overall cost: 7, reverse path:
- 7 distance 7
- 1 distance 6.5
- 2 distance 1.5
- 0 distance 0
3。情节转折:潜伏的逻辑错误?
我对移动 getHeuristic() 更新感到不安。我想知道
我是否可能改变了代码的含义,即使控制
流程似乎已结束。
然后我意识到行为确实发生了变化。这是微妙的。一开始我以为
旧的行为只是有问题。那么,我们来分析一下:
风险的来源是节点访问与队列优先级不一致。
- 访问节点时,查看目标是否变为“less”的条件
距离”仅用距离表示。
- 但是,队列优先级将基于 cost,这是不同的
距离,因为它包括任何启发式方法。
潜伏在那里的问题是有可能编写代码
距离缩短的事实,并不一定保证成本会降低。
回到代码,我们可以看到这是勉强避免的,因为
getHeuristic update 只在代码的非更新路径中执行。
结论
了解这一点让我意识到
-
Vertex::heuristic 字段仅用作getHeuristic() 函数调用的“缓存”版本
- 暗示该函数被视为是幂等的
- 我的版本确实改变了
getHeuristic 现在的行为
可能对同一个顶点执行多次(如果再次访问
通过更便宜的途径)
我建议解决这个问题
- 将
heuristic 字段重命名为cachedHeuristic
- 制作一个
enqueue 函数来封装顶点入队的三个步骤:
- 只需省略端点检查,因为它最多可以消除对该节点的一次
getHeuristic 调用,可能不值得增加复杂性
- 添加注释,指出该代码路径的微妙之处
-
UPDATE 正如在 cmets 中发现的那样,我们还需要逆向
operatione (
dequeue) 对称更新handle 所以它反映了
该节点不再在队列中...
它还使人们了解在调用 Heap::increase 之前添加前置条件 assert 的用处。
最终上市
以上改动
-
封装成一个Graph对象,即
-
还从输入中读取图形,例如:
0 2 1.5
0 3 15
2 4 2.5
2 1 5
1 7 0.5
7 3 0.5
每行包含(源、目标、权重)的位置。
-
一个单独的文件可以包含顶点索引[0, ...)的启发式值,
可选换行符分隔,例如“7 11 99 33 44 55”
-
现在返回到达的节点而不是只返回它的成本
Live On Coliru
#include <boost/heap/fibonacci_heap.hpp>
#include <iostream>
#include <deque>
#include <fstream>
using Cost = double;
struct Vertex;
struct OutEdge { // adjacency from implied source vertex
Vertex* target = nullptr;
Cost weight = 1;
};
namespace bh = boost::heap;
using Node = Vertex*;
struct PrioCompare {
bool operator()(Node a, Node b) const;
};
using MutableQueue = bh::fibonacci_heap<Node, bh::compare<PrioCompare>>;
using Handle = MutableQueue::handle_type;
static const Handle NOHANDLE{}; // for expressive comparisons
static constexpr auto INF = std::numeric_limits<Cost>::infinity();
struct Vertex {
Vertex(Cost d = INF, Cost h = 0) : distance(d), cachedHeuristic(h) {}
Cost distance = INF;
Cost cachedHeuristic = 0;
Handle handle{};
Vertex* predecessor = nullptr;
std::vector<OutEdge> adj;
Cost cost() const { return distance + cachedHeuristic; }
Cost euclideanDistanceTo(Vertex* v);
};
// Now Vertex is a complete type, implement comparison
bool PrioCompare::operator()(Node a, Node b) const {
return a->cost() > b->cost();
}
class Graph {
std::vector<Cost> _heuristics;
Cost getHeuristic(Vertex* v) {
size_t n = id(v);
return n < _heuristics.size() ? _heuristics[n] : 0;
}
void doSomeGraphCreationStuff(Vertex const*) {
// this only creates vertices and edges
}
public:
Graph(std::string edgeFile, std::string heurFile) {
{
std::ifstream stream(heurFile);
_heuristics.assign(std::istream_iterator<Cost>(stream), {});
if (!stream.eof())
throw std::runtime_error("Unexpected heuristics");
}
std::ifstream stream(edgeFile);
size_t src, tgt;
double weight;
while (stream >> src >> tgt >> weight) {
_nodes.resize(std::max({_nodes.size(), src + 1, tgt + 1}));
_nodes[src].adj.push_back({node(tgt), weight});
}
if (!stream.eof())
throw std::runtime_error("Unexpected input");
}
Node search(size_t from, size_t to) {
assert(from < _nodes.size());
assert(to < _nodes.size());
return AStar(node(from), node(to));
}
size_t id(Node node) const {
// ugh, this is just for "pretty output"...
for (size_t i = 0; i < _nodes.size(); ++i) {
if (node == &_nodes[i])
return i;
}
throw std::out_of_range("id");
};
Node node(int id) { return &_nodes.at(id); };
private:
// simple graph data structure with minimal helpers:
std::deque<Vertex> _nodes; // reference stable when growing at the back
// search state
MutableQueue _queue;
void enqueue(Node n) {
assert(n && (n->handle == NOHANDLE));
// get heuristic before insertion!
n->cachedHeuristic = getHeuristic(n);
n->handle = _queue.push(n);
}
Node dequeue() {
Node node = _queue.top();
node->handle = NOHANDLE;
_queue.pop();
return node;
}
Node AStar(Node start, Node destination) {
_queue.clear();
start->distance = 0;
enqueue(start);
while (!_queue.empty()) {
Node u = dequeue();
if (u == destination) {
return u;
}
doSomeGraphCreationStuff(u);
for (auto& [target, weight] : u->adj) {
auto curDistance = u->distance + weight;
// if cheaper route, queue or update queued
if (curDistance < target->distance) {
auto cost_prior = target->cost();
target->distance = curDistance;
target->predecessor = u;
if (target->handle == NOHANDLE) {
// also caches heuristic
enqueue(target);
} else {
// NOTE: avoid updating heuristic here, because it
// breaks the queue invariant if heuristic increased
// more than decrease in distance
assert(target->cost() < cost_prior);
_queue.increase(target->handle);
}
}
}
}
return nullptr;
}
};
int main() {
Graph graph("input.txt", "heur.txt");
Node arrival = graph.search(0, 7);
std::cout << "reverse path: \n";
for (Node n = arrival; n != nullptr; n = n->predecessor) {
std::cout << " - " << graph.id(n) << " cost " << n->cost() << "\n";
}
}
再次打印预期
reverse path:
- 7 cost 7
- 1 cost 17.5
- 2 cost 100.5
- 0 cost 7
注意启发式方法如何改变成本,但在这种情况下不是最优路径。