-
Notifications
You must be signed in to change notification settings - Fork 555
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
Changes from 30 commits
54a199a
b6c88dc
db3b419
fbebb38
891ef72
5f8aff0
c664fb5
560e413
4078af0
4fb5522
84113e5
aa266eb
f087312
fdbe61a
ffee98f
4920e74
5e726b4
aba68a7
fec72f0
2a47958
2c7e465
b7379f4
1b7ac55
9f21d9d
27bdf3a
5343415
f503c15
01bb096
2f63b78
0a7cfa5
e5c4a02
4919472
6395f5b
35df42f
d3350ca
c5824b0
64dcec3
823db45
31bc393
1e40525
d6cfd84
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,7 @@ | |
#include "mcts/node.h" | ||
|
||
#include <algorithm> | ||
#include <bitset> | ||
#include <cassert> | ||
#include <cmath> | ||
#include <cstring> | ||
|
@@ -160,9 +161,60 @@ 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(GameResult result, CertaintyTrigger trigger) { | ||
certainty_state_ |= kCertainMask | kUpperBound | kLowerBound; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! |
||
certainty_state_ &= kGameResultClear; | ||
if (result == GameResult::WHITE_WON) { | ||
certainty_state_ |= kGameResultWin; | ||
} else if (result == GameResult::BLACK_WON) { | ||
certainty_state_ |= kGameResultLoss; | ||
} | ||
if (trigger == CertaintyTrigger::TB_HIT) certainty_state_ |= kTBHit; | ||
if (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; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Extra line break after }. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
} | ||
|
||
|
@@ -197,36 +249,47 @@ 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 { | ||
if (parent_) { | ||
auto edge = parent_->GetEdgeToNode(this); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not a good way to take edge pointing to a node, because:
I think that should be fixed.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this i was never perfectly happy with. I think making the whole code edge centric is a good idea - but maybe thats something that should be done afterwards, as this pertains to a lot of code outside the scope of this PR. Is their a safer way to get to the parent to solve this problem before tackling the edge vs. node centricity of the codebase? |
||
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; | ||
} else if (result == GameResult::WHITE_WON) { | ||
q_ = 1.0f; | ||
} else if (result == GameResult::BLACK_WON) { | ||
q_ = -1.0f; | ||
} | ||
} | ||
|
||
bool Node::TryStartScoreUpdate() { | ||
if (n_ == 0 && n_in_flight_ > 0) return false; | ||
++n_in_flight_; | ||
|
@@ -373,6 +436,8 @@ void NodeTree::MakeMove(Move move) { | |
current_head_->ReleaseChildrenExceptOne(new_head); | ||
current_head_ = | ||
new_head ? new_head : current_head_->CreateSingleChildNode(move); | ||
if (current_head_->GetParent()) current_head_->GetOwnEdge()->ClearCertaintyState(); | ||
current_head_->RecomputeNfromChildren(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is the recompute needed? |
||
history_.Append(move); | ||
} | ||
|
||
|
@@ -417,8 +482,12 @@ bool NodeTree::ResetToPosition(const std::string& starting_fen, | |
// 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(); | ||
if (!seen_old_head) TrimTreeAtHead(); | ||
|
||
// Certainty Propagation: No need to trim the head, just resetting certainty | ||
// state except bounds, and recomputing N should suffices even with WDL hits. | ||
if (current_head_->GetParent()) current_head_->GetOwnEdge()->ClearCertaintyState(); | ||
current_head_->RecomputeNfromChildren(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The rational is this: 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 (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: (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. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
||
|
There was a problem hiding this comment.
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
?There was a problem hiding this comment.
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.....