Skip to content
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.

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

Basic certainty propagation #700

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
54a199a
Efficient and informative depth computation.
Videodr0me Aug 12, 2018
b6c88dc
Update comments and replace tabs with spaces
Videodr0me Aug 13, 2018
db3b419
Merge remote-tracking branch 'upstream/master'
Videodr0me Oct 17, 2018
fbebb38
Merge remote-tracking branch 'upstream/master'
Videodr0me Oct 17, 2018
891ef72
Certainty Propagation
Videodr0me Nov 3, 2018
5f8aff0
Fixes compiler warnings/errors on -pedantic.
Videodr0me Nov 3, 2018
c664fb5
Resolve merge conflicts
Videodr0me Nov 4, 2018
560e413
Resolve Merge Conflicts 2
Videodr0me Nov 4, 2018
4078af0
Merge: unexpected behaviour when go infinite fixed
Videodr0me Nov 4, 2018
4fb5522
Speed fix. Reading non-cached parameters was slow. Now using cached v…
Videodr0me Nov 4, 2018
84113e5
Speed fix. Used reserve in pseudo legal move generation. If compiled …
Videodr0me Nov 5, 2018
aa266eb
Fix for CP=2, CP=2 (default for play) is now more conservative and ad…
Videodr0me Nov 6, 2018
f087312
Bugfixes, codecleanup minor changes:
Videodr0me Dec 1, 2018
fdbe61a
Rename ClearEdge
Videodr0me Dec 1, 2018
ffee98f
change build-cuda to latest
Videodr0me Dec 2, 2018
4920e74
use optional info.mate to display mate scores
Videodr0me Dec 3, 2018
5e726b4
display 0.00 for tablebase draws when syzygy filtered
Videodr0me Dec 4, 2018
aba68a7
Finalize this WIP PR:
Videodr0me Dec 9, 2018
fec72f0
merge with master
Videodr0me Dec 9, 2018
2a47958
Merge remote-tracking branch 'upstream/master'
Videodr0me Dec 14, 2018
2c7e465
Merge remote-tracking branch 'upstream/master'
Videodr0me Dec 14, 2018
b7379f4
Merge branch 'certainty-propagation-negabound'
Videodr0me Jan 18, 2019
1b7ac55
merge with master
Videodr0me Jan 18, 2019
9f21d9d
Prefer terminal wins over certain wins, to avoid delaying mate. Pref…
Videodr0me Jan 26, 2019
27bdf3a
update to master
Videodr0me Jan 26, 2019
5343415
basic certainty propagation - part 2 of PR 487
Videodr0me Jan 26, 2019
f503c15
fix off by one mate count
Videodr0me Jan 27, 2019
01bb096
Make Two Fold Draw Scoring optional to make everybody happy
Videodr0me Jan 29, 2019
2f63b78
fixed typos, comments and formatting
Videodr0me Jan 31, 2019
0a7cfa5
fixed more typos and renamed GetCertaintyStatus to GetCertaintyState
Videodr0me Feb 1, 2019
e5c4a02
ignore .gitignore
Videodr0me Feb 9, 2019
4919472
Cosmetic update and comment update.
Videodr0me Feb 9, 2019
6395f5b
merge to master
Videodr0me Feb 9, 2019
35df42f
fix .gitignore
Videodr0me Feb 10, 2019
d3350ca
remove redundant checks and code formatting
Videodr0me Feb 10, 2019
c5824b0
certain moves without children are reset (n_ = 0), only correct root …
Videodr0me Feb 11, 2019
64dcec3
Streamline node Q getter (now as fast as master - works with PR487!),…
Videodr0me Feb 21, 2019
823db45
Merge branch 'master' into basic-certainty-propagation
Videodr0me Feb 23, 2019
31bc393
merge with master
Videodr0me Feb 23, 2019
1e40525
merge with master2
Videodr0me Feb 23, 2019
d6cfd84
Replace parameter by reference in EvalPosition. Use CertaintyRessult …
Videodr0me Feb 27, 2019
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
2 changes: 2 additions & 0 deletions src/chess/callbacks.h
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ struct ThinkingInfo {
int hashfull = -1;
// Win in centipawns.
optional<int> score;
// Distance to mate.
optional<int> mate;
// Number of successful TB probes (not the same as playouts ending in TB hit).
int tb_hits = -1;
// Best line found. Moves are from perspective of white player.
Expand Down
7 changes: 7 additions & 0 deletions src/chess/position.h
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,13 @@ class Position {
};

enum class GameResult { UNDECIDED, WHITE_WON, DRAW, BLACK_WON };
enum class CertaintyTrigger { NONE, TB_HIT, TWO_FOLD, TERMINAL, NORMAL };
Copy link
Member

Choose a reason for hiding this comment

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

As repetition can be both THREE_FOLD (currently) and TWO_FOLD (probably in the future), how about naming that REPETITION?

Copy link
Collaborator Author

@Videodr0me Videodr0me Feb 10, 2019

Choose a reason for hiding this comment

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

This flag is currently only for two-folds to differentiate them from three-folds. Three-folds are still terminal draws . In a more refined version (not this PR) i used this distinction (all else being equal) to:
(1) Prefer terminal draws (three-folds) if root Q is <0.00 and two-folds if root Q is > 0.00, to allow the opponent make mistakes if we feel we would otherwise be better.

Also it allows to:
(2) Use the two-fold flag to display not 0.00 if best move is a two-fold but the Q from previous (first) time the position arose or root Q. This would solve the issue of displaying 0.00 score before the opponent diverges. SF fsome time ago added something similar to suppress these scores. Its a purely cosmetic issue, but here a flag to separate two-folds from three-folds would also come in handy.

I could rename it to REPETITION, SECOND_REPETITION or add another flag for a THREE_FOLD which could be set in terminals (even though the node being terminal already indicates that its a three_fold, because two_folds do not terminate the game and are just scored differently). Any name is fine by me.....


struct CertaintyResult {
public:
GameResult gameresult;
CertaintyTrigger trigger;
};

class PositionHistory {
public:
Expand Down
8 changes: 7 additions & 1 deletion src/chess/uciloop.cc
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,13 @@ void UciLoop::SendInfo(const std::vector<ThinkingInfo>& infos) {
if (info.seldepth >= 0) res += " seldepth " + std::to_string(info.seldepth);
if (info.time >= 0) res += " time " + std::to_string(info.time);
if (info.nodes >= 0) res += " nodes " + std::to_string(info.nodes);
if (info.score) res += " score cp " + std::to_string(*info.score);

// If mate display mate, otherwise if score display score.
if (info.mate) {
res += " score mate " + std::to_string(*info.mate);
} else if (info.score) {
res += " score cp " + std::to_string(*info.score);
}
if (info.hashfull >= 0) res += " hashfull " + std::to_string(info.hashfull);
if (info.nps >= 0) res += " nps " + std::to_string(info.nps);
if (info.tb_hits >= 0) res += " tbhits " + std::to_string(info.tb_hits);
Expand Down
1 change: 1 addition & 0 deletions src/engine.cc
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,7 @@ void EngineController::Go(const GoParams& params) {
if (info.multipv <= 1) {
ponder_info = info;
if (ponder_info.score) ponder_info.score = -*ponder_info.score;
if (ponder_info.mate) ponder_info.mate = -*ponder_info.mate;
if (ponder_info.depth > 1) ponder_info.depth--;
if (ponder_info.seldepth > 1) ponder_info.seldepth--;
ponder_info.pv.clear();
Expand Down
126 changes: 102 additions & 24 deletions src/mcts/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include "mcts/node.h"

#include <algorithm>
#include <bitset>
#include <cassert>
#include <cmath>
#include <cstring>
Expand Down Expand Up @@ -160,9 +161,58 @@ float Edge::GetP() const {
return ret;
}

void Edge::MakeTerminal(GameResult result) {
certainty_state_ |= kTerminalMask | kCertainMask | kUpperBound | kLowerBound;
certainty_state_ &= kGameResultClear;
if (result == GameResult::WHITE_WON) {
certainty_state_ |= kGameResultWin;
} else if (result == GameResult::BLACK_WON) {
certainty_state_ |= kGameResultLoss;
}
}

void Edge::MakeCertain(CertaintyResult certaintyresult) {
certainty_state_ |= kCertainMask | kUpperBound | kLowerBound;
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider encapsulating result and trigger as a small composite as there are multiple places where both are updated together. Example implementation available over here Videodr0me#3.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If its compatible with PR487, i can do this, but i am not totally convinced it makes the code simpler. But if somebody else weighs in and also finds that this would be nicer, i will change it. Anybody?

Copy link
Contributor

Choose a reason for hiding this comment

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

Especially the EvalPosition func gets easier to read ensuring the certaintyResult is mentioned as a composite of gameresult and certaintyState, although I do understand that mileage on readability may vary. As @ddobbelaere was actively commenting already; could you please have a look at the PR linked above and weigh in?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

certainty_state_ &= kGameResultClear;
if (certaintyresult.gameresult == GameResult::WHITE_WON) {
certainty_state_ |= kGameResultWin;
} else if (certaintyresult.gameresult == GameResult::BLACK_WON) {
certainty_state_ |= kGameResultLoss;
}
if (certaintyresult.trigger == CertaintyTrigger::TB_HIT) certainty_state_ |= kTBHit;
if (certaintyresult.trigger == CertaintyTrigger::TWO_FOLD) certainty_state_ |= kTwoFold;
}

void Edge::MakeCertain(int q, CertaintyTrigger trigger) {
certainty_state_ |= kCertainMask | kUpperBound | kLowerBound;
certainty_state_ &= kGameResultClear;
if (q == 1) {
certainty_state_ |= kGameResultWin;
} else if (q == -1) {
certainty_state_ |= kGameResultLoss;
}
if (trigger == CertaintyTrigger::TB_HIT) certainty_state_ |= kTBHit;
if (trigger == CertaintyTrigger::TWO_FOLD) certainty_state_ |= kTwoFold;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra line break after }.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

void Edge::SetEQ(int eq) {
certainty_state_ &= kGameResultClear;
if (eq == 1) {
certainty_state_ |= kGameResultWin;
} else if (eq == -1) {
certainty_state_ |= kGameResultLoss;
}
}

int Edge::GetEQ() const {
if (certainty_state_ & kGameResultLoss) return -1;
if (certainty_state_ & kGameResultWin) return 1;
return 0;
}

std::string Edge::DebugString() const {
std::ostringstream oss;
oss << "Move: " << move_.as_string() << " p_: " << p_ << " GetP: " << GetP();
oss << "Move: " << move_.as_string() << " p_: " << p_ << " GetP: " << GetP()
<< " Certainty:" << std::bitset<8>(certainty_state_);
return oss.str();
}

Expand Down Expand Up @@ -197,39 +247,51 @@ void Node::CreateEdges(const MoveList& moves) {
Node::ConstIterator Node::Edges() const { return {edges_, &child_}; }
Node::Iterator Node::Edges() { return {edges_, &child_}; }

void Node::RecomputeNfromChildren() {
if (n_ > 1) {
uint32_t visits = 1;
for (const auto& child : Edges()) visits += child.GetN();
n_ = visits;
}
assert(n_in_flight_ == 0);
}

float Node::GetVisitedPolicy() const { return visited_policy_; }

float Node::GetQ() const {
// Currently all certain edges have a corresponding node (PR700) and
// that nodes q_ is set correctly. If we later allow edges to become
// certain without creating the node (PR487 through look-ahead-search),
// we need to revisit this getter or adapt search.
// if (parent_) {
// auto edge = parent_->GetEdgeToNode(this);
// if (edge->IsCertain()) return (float)edge->GetEQ();
// }
return q_;
}

Edge* Node::GetEdgeToNode(const Node* node) const {
assert(node->parent_ == this);
assert(node->index_ < edges_.size());
return &edges_[node->index_];
}

Edge* Node::GetOwnEdge() const { return GetParent()->GetEdgeToNode(this); }
Edge* Node::GetOwnEdge() const {
if (GetParent()) {
return GetParent()->GetEdgeToNode(this);
} else
return nullptr;
}

std::string Node::DebugString() const {
std::ostringstream oss;
oss << " Term:" << is_terminal_ << " This:" << this << " Parent:" << parent_
<< " Index:" << index_ << " Child:" << child_.get()
<< " Sibling:" << sibling_.get() << " Q:" << q_ << " N:" << n_
<< " N_:" << n_in_flight_ << " Edges:" << edges_.size();
oss << " This:" << this << " Parent:" << parent_ << " Index:" << index_
<< " Child:" << child_.get() << " Sibling:" << sibling_.get()
<< " Q:" << q_ << " N:" << n_ << " N_:" << n_in_flight_
<< " Edges:" << edges_.size();
return oss.str();
}

void Node::MakeTerminal(GameResult result) {
is_terminal_ = true;
if (result == GameResult::DRAW) {
q_ = 0.0f;
d_ = 1.0f;
} else if (result == GameResult::WHITE_WON) {
q_ = 1.0f;
d_ = 0.0f;
} else if (result == GameResult::BLACK_WON) {
q_ = -1.0f;
d_ = 0.0f;
}
}

bool Node::TryStartScoreUpdate() {
if (n_ == 0 && n_in_flight_ > 0) return false;
++n_in_flight_;
Expand Down Expand Up @@ -388,6 +450,13 @@ void NodeTree::MakeMove(Move move) {
current_head_->ReleaseChildrenExceptOne(new_head);
current_head_ =
new_head ? new_head : current_head_->CreateSingleChildNode(move);
// If certain and no children, reset node (so that n_ = 0).
if (current_head_->IsCertain() && !current_head_->HasChildren())
TrimTreeAtHead();
// Clear certainty flag but keep bounds.
if (current_head_->GetParent())
current_head_->GetOwnEdge()->ClearCertaintyState();
current_head_->RecomputeNfromChildren();
Copy link
Member

Choose a reason for hiding this comment

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

Why is the recompute needed?
(better to add comment in the code).

history_.Append(move);
}

Expand Down Expand Up @@ -430,10 +499,19 @@ bool NodeTree::ResetToPosition(const std::string& starting_fen,
// previously searched position, which means that the current_head_ might
// retain old n_ and q_ (etc) data, even though its old children were
// previously trimmed; we need to reset current_head_ in that case.
// Also, if the current_head_ is terminal, reset that as well to allow forced
// analysis of WDL hits, or possibly 3 fold or 50 move "draws", etc.
if (!seen_old_head || current_head_->IsTerminal()) TrimTreeAtHead();

// Also, if the current_head_ is certain and has no children, reset that
// as well to allow forced analysis of WDL hits, or possibly 2 or 3 fold
// or 50 move "draws", etc.
if (!seen_old_head ||
(current_head_->IsCertain() && !current_head_->HasChildren()))
TrimTreeAtHead();
// Certainty Propagation: No need to trim the head for certain nodes with
// children (these became certain through backpropagation), just resetting
// certainty state except bounds, and recomputing N suffices. TrimTreeAtHead
// sets n_ to 0 this remains 0 after RecomputeNfromChildren.
if (current_head_->GetParent())
current_head_->GetOwnEdge()->ClearCertaintyState();
current_head_->RecomputeNfromChildren();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not seeing how this works - terminals and certains created at ExtendNode time don't get their edges generated - so when they become the head they have no edges - which breaks the concept of choosing the next move from that position.
If RecomputeNFromChildren could set it to 0 if it has no children, that might work - but since it sets n to 1 it won't be candidate for selection for extension. Generally I think if current_head_ has no child edges, its safer to call TrimTreeAtHead like we used to.

Copy link
Collaborator Author

@Videodr0me Videodr0me Feb 9, 2019

Choose a reason for hiding this comment

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

The rational is this:
(1) certain edges are not only created in ExtendNode but also when backpropagating up the tree
(2) as soon as a normal edge is made certain through backpropagation the children of the node it leads to no longer get visits, while the node itself might still get visits.
(3) Now, if such a node becomes the new head, the sum of its children+ 1 is no longer consisten with its N.

RecomputeNfromChildren ensures this consistency and only changes n if n>1, so for node with no children n is left untouched.

At least thats whats supposed to happen.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I understand what you are trying to do - and it probably makes sense for certain edges that aren't created in ExtendNode - but those that are created in ExtendNode don't get their own edges added, but do still have GetN() returning something other than 0.. If you don't fix that they can't be used as a valid root node.

Copy link
Collaborator Author

@Videodr0me Videodr0me Feb 10, 2019

Choose a reason for hiding this comment

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

I see the following cases potentially arising. Keep in mind that RecomputeNfromChildren only recalculates when the conditional ´if (n_ > 1) ´ is met, otherwise it leaves n_ untouched:

(1) A node is extended, its child edges get created, the NN results are fetched and its score updated. After this n_ = 1 and the conditional (n_ > 1) is not met. If this node becomes root
-> behaviour from normal leela is unaltered, so n_ is correct and consistent

(2) A node is extended, its child edges get createt, but the NN results are not yet fetched. This case probably can't even happen, but to illustrate. If this node gets to be root:
-> behaviour from normal leela is unaltered (as n_ = 0 at that point) , so n_ is correct and consistent.

(3) A (non certain) node has already a visit, and child edges and now gets a second visit that leads to extending one of its childs. Here this node has n_ = 2, and one of the children has n_1. If this node gets to be root,the conditional kicks and n_ is recomputed to the same value n_= 2.
-> so, n_ is correct and consistent.
This should hold for any number of prior visits.

We only really need to Recompute if the node is certain, maybe such a condition could be added for optimization. But apart from that I must be missing something here? Please explain.

Copy link
Contributor

Choose a reason for hiding this comment

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

I added a separate comment to maybe draw your attention to the specific problem more clearly. You seem to be missing the case I was specifically calling out.

return seen_old_head;
}

Expand Down
Loading