Repository: dwavesystems/dwave-optimization (language: python)

Issue:
Consider changing `Symbol.__repr__()` to report underlying node ID rather than python symbol ID
Right now if you have two different symbols pointing to the same underlying C++ node, you get different `repr`s. E.g.,
```python
In [4]: from dwave.optimization import Model

In [5]: model = Model()

In [6]: c0 = model.constant(5)

In [7]: c1, = model.iter_symbols()

In [8]: c0
Out[8]: <dwave.optimization.symbols.Constant at 0x7f5d22e28400>

In [9]: c1
Out[9]: <dwave.optimization.symbols.Constant at 0x7f5d203da3e0>
```
Both of these symbols are referencing the same underlying C++ node, but the `repr` does not reflect that.

We should consider replacing the `0x...` with one pointing to the underlying node pointer.

The cost of course is losing information about the Python object uniqueness, as opposed to the quantity represented uniqueness.


Discussion / hints:

Fix patch:
diff --git a/docs/_images/to_networkx_example.svg b/docs/_images/to_networkx_example.svg
new file mode 100644
index 00000000..a6ca0960
--- /dev/null
+++ b/docs/_images/to_networkx_example.svg
@@ -0,0 +1,1 @@
+<svg baseProfile="full" height="100%" version="1.1" viewBox="25.0,5.0,475.0,120.0" width="100%" xmlns="http://www.w3.org/2000/svg" xmlns:ev="http://www.w3.org/2001/xml-events" xmlns:xlink="http://www.w3.org/1999/xlink"><defs /><g><line stroke="lightgrey" stroke-dasharray="2" x1="90.0" x2="89.0" y1="15.0" y2="15.0" /><line stroke="lightgrey" stroke-dasharray="2" x1="70.0" x2="89.0" y1="35.0" y2="35.0" /><line stroke="lightgrey" stroke-dasharray="2" x1="50.0" x2="89.0" y1="55.0" y2="55.0" /><line stroke="lightgrey" stroke-dasharray="2" x1="50.0" x2="89.0" y1="75.0" y2="75.0" /><line stroke="lightgrey" stroke-dasharray="2" x1="50.0" x2="89.0" y1="95.0" y2="95.0" /><line stroke="lightgrey" stroke-dasharray="2" x1="50.0" x2="49.0" y1="115.0" y2="115.0" /></g><g><line stroke="#53c653" stroke-width="2.0" x1="35.0" x2="35.0" y1="55.0" y2="75.0" /><line stroke="#c6c653" stroke-width="2.0" x1="55.0" x2="55.0" y1="35.0" y2="60.0" /><line stroke="#53c6c6" stroke-width="2.0" x1="35.0" x2="35.0" y1="75.0" y2="95.0" /><line stroke="#c65353" stroke-width="2.0" x1="75.0" x2="75.0" y1="15.0" y2="80.0" /><line stroke="#5353c6" stroke-width="2.0" x1="35.0" x2="35.0" y1="95.0" y2="115.0" /></g><g><line stroke="white" stroke-width="6.0" x1="35.0" x2="40.0" y1="75.0" y2="75.0" /><line stroke="white" stroke-width="6.0" x1="35.0" x2="60.0" y1="95.0" y2="95.0" /></g><g><path d="M 55.0,60.0 a15.0,15.0 0 0,1 -15.0,15.0" fill="none" stroke="#c6c653" stroke-width="2.0" /><line stroke="#c6c653" stroke-width="2.0" x1="35.0" x2="40.0" y1="75.0" y2="75.0" /><path d="M 75.0,80.0 a15.0,15.0 0 0,1 -15.0,15.0" fill="none" stroke="#c65353" stroke-width="2.0" /><line stroke="#c65353" stroke-width="2.0" x1="35.0" x2="60.0" y1="95.0" y2="95.0" /></g><g><circle cx="75.0" cy="15.0" fill="#c65353" id="NdwaveoptimizationsymbolsConstantat0x559b56f604a0" r="6.0" stroke="white" stroke-width="2.0" /><text dominant-baseline="middle" font-family="sans-serif" font-size="inherit" x="95.0" y="15.0">&lt;dwave.optimization.symbols.Constant at 0x559b56f604a0&gt;</text><circle cx="55.0" cy="35.0" fill="#c6c653" id="NdwaveoptimizationsymbolsConstantat0x559b568bd0c0" r="6.0" stroke="white" stroke-width="2.0" /><text dominant-baseline="middle" font-family="sans-serif" font-size="inherit" x="95.0" y="35.0">&lt;dwave.optimization.symbols.Constant at 0x559b568bd0c0&gt;</text><circle cx="35.0" cy="55.0" fill="#53c653" id="NdwaveoptimizationsymbolsIntegerVariableat0x559b56d274d0" r="6.0" stroke="white" stroke-width="2.0" /><text dominant-baseline="middle" font-family="sans-serif" font-size="inherit" x="95.0" y="55.0">&lt;dwave.optimization.symbols.IntegerVariable at 0x559b56d274d0&gt;</text><circle cx="35.0" cy="75.0" fill="#53c6c6" id="NdwaveoptimizationsymbolsMultiplyat0x559b57314190" r="6.0" stroke="white" stroke-width="2.0" /><text dominant-baseline="middle" font-family="sans-serif" font-size="inherit" x="95.0" y="75.0">&lt;dwave.optimization.symbols.Multiply at 0x559b57314190&gt;</text><circle cx="35.0" cy="95.0" fill="#5353c6" id="NdwaveoptimizationsymbolsSubtractat0x559b56ee36f0" r="6.0" stroke="white" stroke-width="2.0" /><text dominant-baseline="middle" font-family="sans-serif" font-size="inherit" x="95.0" y="95.0">&lt;dwave.optimization.symbols.Subtract at 0x559b56ee36f0&gt;</text><circle cx="35.0" cy="115.0" fill="#c653c6" id="Nminimize" r="6.0" stroke="white" stroke-width="2.0" /><text dominant-baseline="middle" font-family="sans-serif" font-size="inherit" x="55.0" y="115.0">minimize</text></g></svg>
\ No newline at end of file
diff --git a/dwave/optimization/model.pyx b/dwave/optimization/model.pyx
index 246257f9..c6279d17 100644
--- a/dwave/optimization/model.pyx
+++ b/dwave/optimization/model.pyx
@@ -40,6 +40,7 @@ import numpy as np
 from cpython cimport Py_buffer
 from cython.operator cimport dereference as deref, preincrement as inc
 from cython.operator cimport typeid
+from libc.stdint cimport uintptr_t
 from libcpp cimport bool
 from libcpp.utility cimport move
 from libcpp.vector cimport vector
@@ -845,53 +846,71 @@ cdef class Model:
     def to_networkx(self):
         """Convert the model to a NetworkX graph.
 
-        Note:
-            Currently requires the installation of a GNU compiler.
-
         Returns:
-            A :obj:`NetworkX <networkx:networkx.Graph>` graph.
+            A :obj:`NetworkX <networkx:networkx.MultiDiGraph>` graph.
 
         Examples:
             This example converts a model to a graph.
 
             >>> from dwave.optimization.model import Model
             >>> model = Model()
-            >>> c = model.constant(8)
-            >>> i = model.integer((20, 30))
-            >>> g = model.to_networkx()   # doctest: +SKIP
-        """
-        # Todo: adapt to use iter_symbols()
-        # This whole method will need a re-write, it currently only works with gcc
-        # but it is useful for development
+            >>> one = model.constant(1)
+            >>> two = model.constant(2)
+            >>> i = model.integer()
+            >>> model.minimize(two * i - one)
+            >>> G = model.to_networkx()
 
-        import re
-        import networkx
+            One advantage of converting to NetworkX is the wide availability
+            of drawing tools. See NetworkX's
+            `drawing <https://networkx.org/documentation/stable/reference/drawing.html>`_
+            documentation.
 
-        G = networkx.DiGraph()
+            This example uses `DAGVIZ <https://wimyedema.github.io/dagviz/>`_ to
+            draw the NetworkX graph created in the example above.
 
-        cdef cppNode* ptr
-        for i in range(self._graph.num_nodes()):
-            ptr = self._graph.nodes()[i].get()
+            >>> import dagviz                      # doctest: +SKIP
+            >>> r = dagviz.render_svg(G)           # doctest: +SKIP
+            >>> with open("model.svg", "w") as f:  # doctest: +SKIP
+            ...     f.write(r)
 
-            # this regex is compiler specific! Don't do this for the general case
-            match = re.search("\d+([a-zA-z]+Node)", str(typeid(deref(ptr)).name()))
-            if not match:
-                raise ValueError
+            This creates the following image:
 
-            u = (match[1], <long>(ptr))
+            .. figure:: /_images/to_networkx_example.svg
+               :width: 500 px
+               :name: dwave-optimization-to-networkx-example
+               :alt: Image of NetworkX Directed Graph
 
-            G.add_node(u)
+        """
+        import networkx
 
-            for j in range(ptr.predecessors().size()):
-                pptr = ptr.predecessors()[j]
+        G = networkx.MultiDiGraph()
 
-                match = re.search("\d+([a-zA-z]+Node)", str(typeid(deref(pptr)).name()))
-                if not match:
-                    raise ValueError
+        # Add the symbols, in order if we happen to be topologically sorted
+        G.add_nodes_from(repr(symbol) for symbol in self.iter_symbols())
 
-                v = (match[1], <long>(pptr))
+        # Sanity check. If several nodes map to the same symbol repr we'll see
+        # too few nodes in the graph
+        if len(G) != self.num_symbols():
+            raise RuntimeError("symbol repr() is not unique to the underlying node")
 
-                G.add_edge(v, u)
+        # Now add the edges
+        for symbol in self.iter_symbols():
+            for successor in symbol.iter_successors():
+                G.add_edge(repr(symbol), repr(successor))
+
+        # Sanity check again. If the repr of symbols isn't unique to the underlying
+        # node then we'll see too many nodes in the graph here
+        if len(G) != self.num_symbols():
+            raise RuntimeError("symbol repr() is not unique to the underlying node")
+
+        # Add the objective if it's present. We call it "minimize" to be
+        # consistent with the minimize() function
+        if self.objective is not None:
+            G.add_edge(repr(self.objective), "minimize")
+
+        # Likewise if we have constraints, add a special node for them
+        for symbol in self.iter_constraints():
+            G.add_edge(repr(symbol), "constraint(s)")
 
         return G
 
@@ -1193,6 +1212,20 @@ cdef class Symbol:
         # via their subclasses.
         raise ValueError("Symbols cannot be constructed directly")
 
+    def __repr__(self):
+        """Return a representation of the symbol.
+
+        The representation refers to the identity of the underlying node, rather than
+        the identity of the Python symbol.
+        """
+        cls = type(self)
+        # We refer to the node_ptr, which is not necessarily the address of the
+        # C++ node, as it sublasses Node.
+        # But this is unique to each node, and functions as an id rather than
+        # as a pointer, so that's OK.
+        # Otherwise we aim to match Python's default __repr__.
+        return f"<{cls.__module__}.{cls.__qualname__} at {<uintptr_t>self.node_ptr:#x}>"
+
     cdef void initialize_node(self, Model model, cppNode* node_ptr) noexcept:
         self.model = model
 
diff --git a/releasenotes/notes/fix-to_networkx-a78f0f669cc9638c.yaml b/releasenotes/notes/fix-to_networkx-a78f0f669cc9638c.yaml
new file mode 100644
index 00000000..fe0a409e
--- /dev/null
+++ b/releasenotes/notes/fix-to_networkx-a78f0f669cc9638c.yaml
@@ -0,0 +1,9 @@
+---
+features:
+  - |
+    Make ``repr()`` of symbols unique to the underlying node, rather than to the Python symbol.
+    See `#52 <https://github.com/dwavesystems/dwave-optimization/issues/52>_.
+fixes:
+  - |
+    Fix ``Symbol.to_networkx()`` to no longer be compiler-dependant.
+    See `#18 <https://github.com/dwavesystems/dwave-optimization/issues/18>_.
\ No newline at end of file


Test patch:
diff --git a/tests/test_model.py b/tests/test_model.py
index 232cdd4c..3565ed47 100644
--- a/tests/test_model.py
+++ b/tests/test_model.py
@@ -260,6 +260,76 @@ def test_state_size(self):
         model.constant(np.arange(25).reshape(5, 5))
         self.assertEqual(model.state_size(), 25 * 8)
 
+    def test_to_networkx(self):
+        try:
+            import networkx as nx
+        except ImportError:
+            return self.skipTest("NetworkX is not installed")
+
+        model = Model()
+        a = model.binary()
+        b = model.binary()
+        ab = a * b
+
+        G = model.to_networkx()
+
+        self.assertEqual(len(G.nodes), 3)
+        self.assertEqual(len(G.edges), 2)
+
+        # the repr as labels is an implementation detail and subject to change
+        self.assertIn(repr(a), G.nodes)
+        self.assertIn(repr(b), G.nodes)
+        self.assertIn(repr(ab), G.nodes)
+        self.assertIn((repr(a), repr(ab)), G.edges)
+        self.assertIn((repr(b), repr(ab)), G.edges)
+
+        # graph created is deterministic
+        self.assertTrue(nx.utils.graphs_equal(G, model.to_networkx()))
+
+    def test_to_networkx_multigraph(self):
+        try:
+            import networkx as nx
+        except ImportError:
+            return self.skipTest("NetworkX is not installed")
+
+        model = Model()
+        a = model.binary()
+        aa = a * a  # two edges to the same node
+
+        G = model.to_networkx()
+
+        # the repr as labels is an implementation detail and subject to change
+        self.assertEqual(len(G.nodes), 2)
+        self.assertEqual(len(G.edges), 2)
+        self.assertIn(repr(a), G.nodes)
+        self.assertIn(repr(aa), G.nodes)
+        self.assertIn((repr(a), repr(aa)), G.edges)
+
+        # graph created is deterministic
+        self.assertTrue(nx.utils.graphs_equal(G, model.to_networkx()))
+
+    def test_to_networkx_objective_and_constraints(self):
+        try:
+            import networkx as nx
+        except ImportError:
+            return self.skipTest("NetworkX is not installed")
+
+        model = Model()
+        a = model.binary()
+        b = model.binary()
+        model.minimize(a * b)
+
+        G = model.to_networkx()
+        self.assertEqual(len(G.nodes), 4)  # 3 symbols + "minimize"
+        self.assertEqual(len(G.edges), 3)
+        self.assertIn("minimize", G.nodes)
+
+        model.add_constraint(a <= b)
+        G = model.to_networkx()
+        self.assertEqual(len(G.nodes), 6)
+        self.assertEqual(len(G.edges), 6)
+        self.assertIn("constraint(s)", G.nodes)
+
 
 class TestStates(unittest.TestCase):
     def test_clear(self):
@@ -424,3 +494,14 @@ def test_abstract(self):
         from dwave.optimization.model import Symbol
         with self.assertRaisesRegex(ValueError, "Symbols cannot be constructed directly"):
             Symbol()
+
+    def test_repr(self):
+        model = Model()
+        c0 = model.constant(5)
+        c1, = model.iter_symbols()
+        c2 = model.constant(5)
+
+        # the specific form is an implementation detail, but different symbols
+        # representing the same underlying node should have the same repr
+        self.assertEqual(repr(c0), repr(c1))
+        self.assertNotEqual(repr(c0), repr(c2))