Conversation
tturocy
left a comment
tturocy
left a comment
There was a problem hiding this comment.
Aside from (or perhaps related to) the specific comments, I'm concerned this is mixing together logically different changes. This will make it hard to review, but also down the road makes it harder to debug if these changes introduce bugs.
For example, having sequence-related functions in the game and making them available in Python is logically distinct from touching the LP/LCP solvers.
| @cython.cfunc | ||
| def rat_to_py(r: c_Rational): | ||
| """Convert a C++ Rational number to a Python Rational.""" | ||
| return Rational(to_string(r).decode("ascii")) |
There was a problem hiding this comment.
I'm not sure I like this change. A rational will never decode to an empty string, so if this happens there is an error somewhere. This change will just hide that error.
|
|
||
| virtual std::shared_ptr<BehaviorSupportProfile> GetFullSupport() | ||
| { | ||
| throw std::runtime_error("Sequence form can only be generated for extensive form games"); |
There was a problem hiding this comment.
We already have a not implemented exception which is used for this purpose.
| mutable std::set<GameInfosetRep *> m_absentMindedInfosets; | ||
| mutable std::shared_ptr<BehaviorSupportProfile> m_fullSupport; | ||
|
|
||
| std::shared_ptr<BehaviorSupportProfile> GetFullSupport() override |
There was a problem hiding this comment.
This is not the way to do this. We should be cacheing a sequence form, not a full support.
| auto player2 = p_game->GetPlayer(2); | ||
| auto sequences1 = p_game->GetSequences(player1); | ||
| auto sequences2 = p_game->GetSequences(player2); | ||
| for (auto seq : sequences1) { |
There was a problem hiding this comment.
This is definitely where the refactoring work is starting to pay off, as this is getting closer to being much easier to read/understand!
| @@ -67,22 +67,26 @@ void GameSequenceForm::BuildSequences() | |||
| } | |||
|
|
|||
| void GameSequenceForm::FillTableau(const GameNode &n, const Rational &prob, | |||
There was a problem hiding this comment.
We definitely need to deal with this recursion and convert it to a stack-based method. But that is outside the scope of this task.
| Sequences::iterator Sequences::begin() const { return {m_support->GetSequenceForm(), false}; } | ||
| Sequences::iterator Sequences::end() const { return {m_support->GetSequenceForm(), true}; } | ||
|
|
||
| Sequences::iterator::iterator(const std::shared_ptr<GameSequenceForm> p_sfg, bool p_end) |
There was a problem hiding this comment.
We have a NestedElementCollection which implements exactly this generically - see how Infosets and Strategies are implemented.
2 participants
Updated LP and LCP solvers, in doing so fixing a bug involving internal and missing outcomes (Issue #654). Sequence form object has been updated so that payoffs are on terminal nodes. Sequence form is now an attribute of a Game object. In addition, several tests have been added.