Correct validation of action probabilities. (#798)

This fixes a problem first observed in the graphical interface (#788), in which
validation of chance probabilities was not being handled correctly.

The slightly deeper issue was that validation was not exposed separately.

The solution is to refactor and expose a validation function, so the validation logic is available
both in setting the probabilities, but also in validating the dialog.

This also adds a few small UX and wxWidgets modernisations.

Fixes #788.

Author: tturocy

src/games/game.h

@@ -593,6 +593,21 @@ inline GameNodeRep::Actions::iterator::iterator(GameInfosetRep::Actions::iterato
 
 inline GameNode GameNodeRep::Actions::iterator::GetOwner() const { return m_child_it.GetOwner(); }
 
+inline void ValidateDistribution(const Array<Number> &p_probs, const bool p_normalized = true)
+{
+  if (std::any_of(p_probs.begin(), p_probs.end(),
+                  [](const Number &x) { return static_cast<Rational>(x) < Rational(0); })) {
+    throw ValueException("Probabilities must be non-negative numbers");
+  }
+  if (!p_normalized) {
+    return;
+  }
+  if (sum_function(p_probs, [](const Number &n) { return static_cast<Rational>(n); }) !=
+      Rational(1)) {
+    throw ValueException("Probabilities must sum to exactly one");
+  }
+}
+
 enum class TraversalOrder { Preorder, Postorder };
 
 class CartesianProductSpace {

src/games/gametree.cc

@@ -1324,17 +1324,8 @@ Game GameTreeRep::SetChanceProbs(const GameInfoset &p_infoset, const Array<Numbe
   if (p_infoset->m_actions.size() != p_probs.size()) {
     throw DimensionException("The number of probabilities given must match the number of actions");
   }
+  ValidateDistribution(p_probs);
   IncrementVersion();
-  if (std::any_of(p_probs.begin(), p_probs.end(),
-                  [](const Number &x) { return static_cast<Rational>(x) < Rational(0); })) {
-    throw ValueException("Probabilities must be non-negative numbers");
-  }
-  auto sum = std::accumulate(
-      p_probs.begin(), p_probs.end(), Rational(0),
-      [](const Rational &r, const Number &n) { return r + static_cast<Rational>(n); });
-  if (sum != Rational(1)) {
-    throw ValueException("Probabilities must sum to exactly one");
-  }
   std::copy(p_probs.begin(), p_probs.end(), p_infoset->m_probs.begin());
   ClearComputedValues();
   return shared_from_this();

src/gui/dleditmove.cc

@@ -32,8 +32,11 @@
 #include "renratio.h"
 
 namespace Gambit::GUI {
+
 class ActionSheet final : public wxSheet {
-  GameInfoset m_infoset;
+  GameInfoset m_infoset{nullptr};
+  wxSheetCellAttr m_labelAttr;
+  wxFont m_labelFont, m_cellFont;
 
   // Overriding wxSheet members
   wxSheetCellAttr GetAttr(const wxSheetCoords &p_coords, wxSheetAttr_Type) const override;
@@ -48,8 +51,17 @@ class ActionSheet final : public wxSheet {
 };
 
 ActionSheet::ActionSheet(wxWindow *p_parent, const GameInfoset &p_infoset)
-  : wxSheet(p_parent, wxID_ANY), m_infoset(p_infoset)
+  : wxSheet(p_parent, wxID_ANY), m_infoset(p_infoset), m_labelFont(GetFont()),
+    m_cellFont(GetFont())
 {
+  m_labelFont.MakeBold();
+
+  m_labelAttr = GetSheetRefData()->m_defaultRowLabelAttr;
+  m_labelAttr.SetFont(m_labelFont);
+  m_labelAttr.SetAlignment(wxALIGN_CENTER, wxALIGN_CENTER);
+  m_labelAttr.SetOrientation(wxHORIZONTAL);
+  m_labelAttr.SetReadOnly(true);
+
   CreateGrid(p_infoset->GetActions().size(), (p_infoset->IsChanceInfoset()) ? 2 : 1);
   SetRowLabelWidth(40);
   SetColLabelHeight(25);
@@ -103,29 +115,12 @@ Array<Number> ActionSheet::GetActionProbs()
 
 wxSheetCellAttr ActionSheet::GetAttr(const wxSheetCoords &p_coords, wxSheetAttr_Type) const
 {
-  if (IsRowLabelCell(p_coords)) {
-    wxSheetCellAttr attr(GetSheetRefData()->m_defaultRowLabelAttr);
-    attr.SetFont(wxFont(10, wxFONTFAMILY_SWISS, wxFONTSTYLE_NORMAL, wxFONTWEIGHT_BOLD));
-    attr.SetAlignment(wxALIGN_CENTER, wxALIGN_CENTER);
-    attr.SetOrientation(wxHORIZONTAL);
-    attr.SetReadOnly(true);
-    return attr;
-  }
-  else if (IsColLabelCell(p_coords)) {
-    wxSheetCellAttr attr(GetSheetRefData()->m_defaultColLabelAttr);
-    attr.SetFont(wxFont(10, wxFONTFAMILY_SWISS, wxFONTSTYLE_NORMAL, wxFONTWEIGHT_BOLD));
-    attr.SetAlignment(wxALIGN_CENTER, wxALIGN_CENTER);
-    attr.SetOrientation(wxHORIZONTAL);
-    attr.SetReadOnly(true);
-    return attr;
-  }
-  else if (IsCornerLabelCell(p_coords)) {
-    const wxSheetCellAttr attr(GetSheetRefData()->m_defaultCornerLabelAttr);
-    return attr;
+  if (IsLabelCell(p_coords)) {
+    return m_labelAttr;
   }
 
   wxSheetCellAttr attr(GetSheetRefData()->m_defaultGridCellAttr);
-  attr.SetFont(wxFont(10, wxFONTFAMILY_SWISS, wxFONTSTYLE_NORMAL, wxFONTWEIGHT_NORMAL));
+  attr.SetFont(m_cellFont);
   attr.SetAlignment(wxALIGN_CENTER, wxALIGN_CENTER);
   attr.SetOrientation(wxHORIZONTAL);
   attr.SetReadOnly(false);
@@ -143,9 +138,7 @@ wxSheetCellAttr ActionSheet::GetAttr(const wxSheetCoords &p_coords, wxSheetAttr_
 //                      class EditMoveDialog
 //======================================================================
 
-wxBEGIN_EVENT_TABLE(EditMoveDialog, wxDialog) EVT_BUTTON(wxID_OK, EditMoveDialog::OnOK)
-    wxEND_EVENT_TABLE() EditMoveDialog::EditMoveDialog(wxWindow *p_parent,
-                                                       const GameInfoset &p_infoset)
+EditMoveDialog::EditMoveDialog(wxWindow *p_parent, const GameInfoset &p_infoset)
   : wxDialog(p_parent, wxID_ANY, _("Move properties"), wxDefaultPosition), m_infoset(p_infoset)
 {
   auto *topSizer = new wxBoxSizer(wxVERTICAL);
@@ -171,6 +164,7 @@ wxBEGIN_EVENT_TABLE(EditMoveDialog, wxDialog) EVT_BUTTON(wxID_OK, EditMoveDialog
   if (p_infoset->IsChanceInfoset()) {
     m_player->Append(_("Chance"));
     m_player->SetSelection(0);
+    m_player->Disable();
   }
   else {
     for (const auto &player : p_infoset->GetGame()->GetPlayers()) {
@@ -189,18 +183,13 @@ wxBEGIN_EVENT_TABLE(EditMoveDialog, wxDialog) EVT_BUTTON(wxID_OK, EditMoveDialog
   actionBoxSizer->Add(m_actionSheet, 1, wxALL | wxEXPAND, 5);
   topSizer->Add(actionBoxSizer, 0, wxALL | wxEXPAND, 5);
 
-  auto *buttonSizer = new wxBoxSizer(wxHORIZONTAL);
-  buttonSizer->Add(new wxButton(this, wxID_CANCEL, _("Cancel")), 0, wxALL, 5);
-  auto *okButton = new wxButton(this, wxID_OK, _("OK"));
-  okButton->SetDefault();
-  buttonSizer->Add(okButton, 0, wxALL, 5);
-  topSizer->Add(buttonSizer, 0, wxALL | wxALIGN_RIGHT, 5);
-
-  SetSizer(topSizer);
-  topSizer->Fit(this);
-  topSizer->SetSizeHints(this);
-  wxTopLevelWindowBase::Layout();
+  if (auto *buttons = CreateSeparatedButtonSizer(wxOK | wxCANCEL)) {
+    topSizer->Add(buttons, 0, wxALL | wxEXPAND, 5);
+  }
+  SetSizerAndFit(topSizer);
   CenterOnParent();
+
+  Bind(wxEVT_BUTTON, &EditMoveDialog::OnOK, this, wxID_OK);
 }
 
 void EditMoveDialog::OnOK(wxCommandEvent &p_event)
@@ -209,23 +198,16 @@ void EditMoveDialog::OnOK(wxCommandEvent &p_event)
     p_event.Skip();
     return;
   }
-  auto probs = m_actionSheet->GetActionProbs();
-  if (std::any_of(probs.begin(), probs.end(),
-                  [](const Number &p) { return static_cast<Rational>(p) < Rational(0); })) {
-    wxMessageBox("Action probabilities must be non-negative numbers.", "Error");
-    return;
-  }
-  if (std::accumulate(probs.begin(), probs.end(), Rational(0),
-                      [](const Rational &s, const Number &p) {
-                        return s + static_cast<Rational>(p);
-                      }) != Rational(1)) {
-    p_event.Skip();
+  try {
+    ValidateDistribution(m_actionSheet->GetActionProbs());
   }
-  else {
-    wxRichMessageDialog(this, "Action probabilities must sum to exactly one", "Error",
+  catch (ValueException &) {
+    wxRichMessageDialog(this, "Probabilities must be nonnegative numbers summing to one.", "Error",
                         wxOK | wxCENTRE | wxICON_ERROR)
         .ShowModal();
+    return;
   }
+  p_event.Skip();
 }
 
 int EditMoveDialog::NumActions() const { return m_actionSheet->NumActions(); }

src/gui/dleditmove.h

@@ -45,8 +45,6 @@ class EditMoveDialog final : public wxDialog {
   int NumActions() const;
   wxString GetActionName(int p_act) const;
   Array<Number> GetActionProbs() const;
-
-  wxDECLARE_EVENT_TABLE();
 };
 } // namespace Gambit::GUI