From 78359ad74718158a38377ccd6cd0155398ba252b Mon Sep 17 00:00:00 2001
From: Ben Cumming <bcumming@cscs.ch>
Date: Wed, 12 Jun 2019 13:14:47 +0200
Subject: [PATCH] Uniform str() and repr() in python type wrappers (#785)

Fixes #782
* consistent formatting of strings returned by `__repr__` and `__str__` for wrapped types
* rename enum wrappers to be less verbose, where it makes sense.
---
 python/context.cpp                            | 15 +++++-----
 python/domain_decomposition.cpp               | 18 ++++--------
 python/identifiers.cpp                        | 28 ++++++++-----------
 python/mpi.cpp                                |  4 +--
 python/recipe.cpp                             |  6 ++--
 python/schedule.cpp                           |  6 ++--
 python/simulation.cpp                         |  2 +-
 python/test/unit/test_event_generators.py     | 12 ++------
 python/test/unit/test_identifiers.py          |  8 ++----
 .../unit_distributed/test_contexts_arbmpi.py  |  2 +-
 .../unit_distributed/test_contexts_mpi4py.py  |  2 +-
 11 files changed, 40 insertions(+), 63 deletions(-)

diff --git a/python/context.cpp b/python/context.cpp
index f871d3d5..db1b74e4 100644
--- a/python/context.cpp
+++ b/python/context.cpp
@@ -21,13 +21,13 @@ namespace pyarb {
 
 std::ostream& operator<<(std::ostream& o, const context_shim& ctx) {
     auto& c = ctx.context;
-    const bool gpu = arb::has_gpu(c);
-    const bool mpi = arb::has_mpi(c);
+    const char* gpu = arb::has_gpu(c)? "True": "False";
+    const char* mpi = arb::has_mpi(c)? "True": "False";
     return
-        o << "<context: threads " << arb::num_threads(c)
-          << ", gpu " << (gpu? "yes": "no")
-          << ", mpi " << (mpi? "yes": "no")
-          << ", ranks " << arb::num_ranks(c)
+        o << "<arbor.context: num_threads " << arb::num_threads(c)
+          << ", has_gpu " << gpu
+          << ", has_mpi " << mpi
+          << ", num_ranks " << arb::num_ranks(c)
           << ">";
 }
 
@@ -64,8 +64,7 @@ struct proc_allocation_shim {
 };
 
 std::ostream& operator<<(std::ostream& o, const proc_allocation_shim& alloc) {
-    return o << "<hardware resource allocation: threads " << alloc.num_threads
-      << ", gpu id " << alloc.gpu_id << ">";
+    return o << "<arbor.proc_allocation: threads " << alloc.num_threads << ", gpu_id " << alloc.gpu_id << ">";
 }
 
 void register_contexts(pybind11::module& m) {
diff --git a/python/domain_decomposition.cpp b/python/domain_decomposition.cpp
index db0e6dd4..4a1cefaa 100644
--- a/python/domain_decomposition.cpp
+++ b/python/domain_decomposition.cpp
@@ -14,21 +14,15 @@
 namespace pyarb {
 
 std::string gd_string(const arb::group_description& g) {
-    std::stringstream s;
-    s << "<cell group: " << g.gids.size()
-      << " cells, gids [" << util::csv(g.gids, 5) << "]"
-      << ", " << g.kind << ", " << g.backend << ">";
-    return s.str();
+    return util::pprintf(
+        "<arbor.group_description: num_cells {}, gids [{}], {}, {}",
+        g.gids.size(), util::csv(g.gids, 5), g.kind, g.backend);
 }
 
 std::string dd_string(const arb::domain_decomposition& d) {
-    std::stringstream s;
-    s << "<domain decomposition: domain "
-      << d.domain_id << " of "
-      << d.num_domains << ", "
-      << d.num_local_cells << "/" << d.num_global_cells << " loc/glob cells, "
-      << d.groups.size() << " groups>";
-    return s.str();
+    return util::pprintf(
+        "<arbor.domain_decomposition: domain_id {}, num_domains {}, num_local_cells {}, num_global_cells {}, groups {}>",
+        d.domain_id, d.num_domains, d.num_local_cells, d.num_global_cells, d.groups.size());
 }
 
 void register_domain_decomposition(pybind11::module& m) {
diff --git a/python/identifiers.cpp b/python/identifiers.cpp
index 86888b84..11895074 100644
--- a/python/identifiers.cpp
+++ b/python/identifiers.cpp
@@ -17,30 +17,24 @@ void register_identifiers(pybind11::module& m) {
     pybind11::class_<arb::cell_member_type> cell_member(m, "cell_member",
         "For global identification of a cell-local item.\n\n"
         "Items of cell_member must:\n"
-        "(1) be associated with a unique cell, identified by the member gid;\n"
-        "(2) identify an item within a cell-local collection by the member index.\n");
+        "  (1) be associated with a unique cell, identified by the member gid;\n"
+        "  (2) identify an item within a cell-local collection by the member index.\n");
 
     cell_member
-        .def(pybind11::init<>(),
-            "Construct a cell member with default values gid = 0 and index = 0.")
         .def(pybind11::init(
             [](arb::cell_gid_type gid, arb::cell_lid_type idx) {
-                arb::cell_member_type m;
-                m.gid = gid;
-                m.index = idx;
-                return m;
+                return arb::cell_member_type{gid, idx};
             }),
-            "gid"_a,
-            "index"_a,
-            "Construct a cell member with arguments:/n"
-               "  gid:     The global identifier of the cell.\n"
-               "  index:   The cell-local index of the item.\n")
+            "gid"_a, "index"_a,
+            "Construct a cell member with arguments:\n"
+            "  gid:     The global identifier of the cell.\n"
+            "  index:   The cell-local index of the item.\n")
         .def_readwrite("gid",   &arb::cell_member_type::gid,
             "The global identifier of the cell.")
         .def_readwrite("index", &arb::cell_member_type::index,
             "Cell-local index of the item.")
-        .def("__str__", [](arb::cell_member_type m) {return pprintf("<cell member: gid {}, index {}>", m.gid, m.index);})
-        .def("__repr__",[](arb::cell_member_type m) {return pprintf("<cell member: gid {}, index {}>", m.gid, m.index);});
+        .def("__str__", [](arb::cell_member_type m) {return pprintf("<arbor.cell_member: gid {}, index {}>", m.gid, m.index);})
+        .def("__repr__",[](arb::cell_member_type m) {return pprintf("<arbor.cell_member: gid {}, index {}>", m.gid, m.index);});
 
     pybind11::enum_<arb::cell_kind>(m, "cell_kind",
         "Enumeration used to identify the cell kind, used by the model to group equal kinds in the same cell group.")
@@ -53,14 +47,14 @@ void register_identifiers(pybind11::module& m) {
         .value("spike_source", arb::cell_kind::spike_source,
             "Proxy cell that generates spikes from a spike sequence provided by the user.");
 
-    pybind11::enum_<arb::backend_kind>(m, "backend_kind",
+    pybind11::enum_<arb::backend_kind>(m, "backend",
         "Enumeration used to indicate which hardware backend to use for running a cell_group.")
         .value("gpu", arb::backend_kind::gpu,
             "Use GPU backend.")
         .value("multicore", arb::backend_kind::multicore,
             "Use multicore backend.");
 
-    pybind11::enum_<arb::binning_kind>(m, "binning_kind",
+    pybind11::enum_<arb::binning_kind>(m, "binning",
         "Enumeration for event time binning policy.")
         .value("none", arb::binning_kind::none,
             "No binning policy.")
diff --git a/python/mpi.cpp b/python/mpi.cpp
index 6afb861a..57ea410b 100644
--- a/python/mpi.cpp
+++ b/python/mpi.cpp
@@ -88,10 +88,10 @@ int mpi_is_finalized() {
 
 std::ostream& operator<<(std::ostream& o, const mpi_comm_shim& c) {
     if (c.comm==MPI_COMM_WORLD) {
-        return o << "<mpi communicator: MPI_COMM_WORLD>";
+        return o << "<arbor.mpi_comm: MPI_COMM_WORLD>";
     }
     else {
-        return o << "<mpi communicator: " << c.comm << ">";
+        return o << "<arbor.mpi_comm: " << c.comm << ">";
     }
 }
 
diff --git a/python/recipe.cpp b/python/recipe.cpp
index 17d071b7..fc1f4569 100644
--- a/python/recipe.cpp
+++ b/python/recipe.cpp
@@ -115,12 +115,12 @@ struct cell_connection_shim {
 // TODO: implement py_recipe_shim::probe_info
 
 std::string con_to_string(const cell_connection_shim& c) {
-    return util::pprintf("<connection: ({},{}) -> ({},{}), delay {}, weight {}>",
+    return util::pprintf("<arbor.connection: source ({},{}), destination ({},{}), delay {}, weight {}>",
          c.source.gid, c.source.index, c.destination.gid, c.destination.index, c.delay, c.weight);
 }
 
 std::string gj_to_string(const arb::gap_junction_connection& gc) {
-    return util::pprintf("<gap junction: ({},{}) <-> ({},{}), conductance {}>",
+    return util::pprintf("<arbor.gap_junction_connection: local ({},{}), peer ({},{}), ggap {}>",
          gc.local.gid, gc.local.index, gc.peer.gid, gc.peer.index, gc.ggap);
 }
 
@@ -128,7 +128,7 @@ void register_recipe(pybind11::module& m) {
     using namespace pybind11::literals;
 
     // Connections
-    pybind11::class_<cell_connection_shim> cell_connection(m, "cell_connection",
+    pybind11::class_<cell_connection_shim> cell_connection(m, "connection",
         "Describes a connection between two cells:\n"
         "  Defined by source and destination end points (that is pre-synaptic and post-synaptic respectively), a connection weight and a delay time.");
     cell_connection
diff --git a/python/schedule.cpp b/python/schedule.cpp
index d2b8449c..ee8f81cd 100644
--- a/python/schedule.cpp
+++ b/python/schedule.cpp
@@ -11,19 +11,19 @@
 namespace pyarb {
 
 std::ostream& operator<<(std::ostream& o, const regular_schedule_shim& x) {
-    return o << "<regular_schedule: tstart "
+    return o << "<arbor.regular_schedule: tstart "
              << x.tstart << " ms, dt "
              << x.dt << " ms, tstop "
              << x.tstop << " ms>";
 }
 
 std::ostream& operator<<(std::ostream& o, const explicit_schedule_shim& e) {
-    o << "<explicit_schedule: times [";
+    o << "<arbor.explicit_schedule: times [";
     return o << util::csv(e.times) << "] ms>";
 };
 
 std::ostream& operator<<(std::ostream& o, const poisson_schedule_shim& p) {
-    return o << "<poisson_schedule: tstart " << p.tstart << " ms"
+    return o << "<arbor.poisson_schedule: tstart " << p.tstart << " ms"
              << ", freq " << p.freq << " Hz"
              << ", seed " << p.seed << ">";
 };
diff --git a/python/simulation.cpp b/python/simulation.cpp
index 3b3f4736..07af633b 100644
--- a/python/simulation.cpp
+++ b/python/simulation.cpp
@@ -37,7 +37,7 @@ void register_simulation(pybind11::module& m) {
         .def("set_binning_policy", &arb::simulation::set_binning_policy,
             "Set event binning policy on all our groups.",
             "policy"_a, "bin_interval"_a)
-        .def("__str__", [](const arb::simulation&){ return "<arbor.simulation>"; })
+        .def("__str__",  [](const arb::simulation&){ return "<arbor.simulation>"; })
         .def("__repr__", [](const arb::simulation&){ return "<arbor.simulation>"; });
 }
 
diff --git a/python/test/unit/test_event_generators.py b/python/test/unit/test_event_generators.py
index 527564da..4099d461 100644
--- a/python/test/unit/test_event_generators.py
+++ b/python/test/unit/test_event_generators.py
@@ -22,9 +22,7 @@ all tests for event generators (regular, explicit, poisson)
 class EventGenerator(unittest.TestCase):
 
     def test_event_generator_regular_schedule(self):
-        cm = arb.cell_member()
-        cm.gid = 42
-        cm.index = 3
+        cm = arb.cell_member(42,3)
         rs = arb.regular_schedule(2.0, 1., 100.)
         rg = arb.event_generator(cm, 3.14, rs)
         self.assertEqual(rg.target.gid, 42)
@@ -32,9 +30,7 @@ class EventGenerator(unittest.TestCase):
         self.assertAlmostEqual(rg.weight, 3.14)
 
     def test_event_generator_explicit_schedule(self):
-        cm = arb.cell_member()
-        cm.gid = 0
-        cm.index = 42
+        cm = arb.cell_member(0,42)
         es = arb.explicit_schedule([0,1,2,3,4.4])
         eg = arb.event_generator(cm, -0.01, es)
         self.assertEqual(eg.target.gid, 0)
@@ -42,9 +38,7 @@ class EventGenerator(unittest.TestCase):
         self.assertAlmostEqual(eg.weight, -0.01)
 
     def test_event_generator_poisson_schedule(self):
-        cm = arb.cell_member()
-        cm.gid = 4
-        cm.index = 2
+        cm = arb.cell_member(4,2)
         ps = arb.poisson_schedule(0., 10., 0)
         pg = arb.event_generator(cm, 42., ps)
         self.assertEqual(pg.target.gid, 4)
diff --git a/python/test/unit/test_identifiers.py b/python/test/unit/test_identifiers.py
index 4bd41794..587dd293 100644
--- a/python/test/unit/test_identifiers.py
+++ b/python/test/unit/test_identifiers.py
@@ -20,10 +20,6 @@ all tests for identifiers, indexes, kinds
 """
 
 class CellMembers(unittest.TestCase):
-    def test_default_cell_member(self):
-        cm = arb.cell_member()
-        self.assertEqual(cm.gid, 0)
-        self.assertEqual(cm.index, 0)
 
     def test_gid_index_contor_cell_member(self):
         cm = arb.cell_member(17,42)
@@ -31,10 +27,10 @@ class CellMembers(unittest.TestCase):
         self.assertEqual(cm.index, 42)
 
     def test_set_git_index_cell_member(self):
-        cm = arb.cell_member()
+        cm = arb.cell_member(0,0)
         cm.gid = 13
         cm.index = 23
-        self.assertEqual(cm.gid,13)
+        self.assertEqual(cm.gid, 13)
         self.assertEqual(cm.index, 23)
 
 def suite():
diff --git a/python/test/unit_distributed/test_contexts_arbmpi.py b/python/test/unit_distributed/test_contexts_arbmpi.py
index c999c343..57d15bdb 100644
--- a/python/test/unit_distributed/test_contexts_arbmpi.py
+++ b/python/test/unit_distributed/test_contexts_arbmpi.py
@@ -44,7 +44,7 @@ class Contexts_arbmpi(unittest.TestCase):
         comm = arb.mpi_comm()
 
         # test that by default communicator is MPI_COMM_WORLD
-        self.assertEqual(str(comm), '<mpi communicator: MPI_COMM_WORLD>')
+        self.assertEqual(str(comm), '<arbor.mpi_comm: MPI_COMM_WORLD>')
 
     def test_context_arbmpi(self):
         comm = arb.mpi_comm()
diff --git a/python/test/unit_distributed/test_contexts_mpi4py.py b/python/test/unit_distributed/test_contexts_mpi4py.py
index 2a58c8bc..7a05bb5a 100644
--- a/python/test/unit_distributed/test_contexts_mpi4py.py
+++ b/python/test/unit_distributed/test_contexts_mpi4py.py
@@ -37,7 +37,7 @@ class Contexts_mpi4py(unittest.TestCase):
         comm = arb.mpi_comm(mpi.COMM_WORLD)
 
         # test that set communicator is MPI_COMM_WORLD
-        self.assertEqual(str(comm), '<mpi communicator: MPI_COMM_WORLD>')
+        self.assertEqual(str(comm), '<arbor.mpi_comm: MPI_COMM_WORLD>')
 
     def test_context_mpi4py(self):
         comm = arb.mpi_comm(mpi.COMM_WORLD)
-- 
GitLab