NotImplementedError for immutable graphs

[Jonathan Kliem]

Dear all,

when a graph is immutable, there are meaningless error messages that make it hard for the user to guess what is going on (see below).

When I create graphs like this, I know perfectly well what is going on. However, when this is done in a cached method, it is hard for the user to figure out the problem, isn't it?

I'm currently working on #28828 and returning an immutable graph seems to leave the user clueless about why the code breaks. Of course one could just cache a private method and return a copy or manually cache it, but that seems to be against the point of @cached_method.

Any suggestion for a good fix? Am I missing something?

Thanks,

Jonathan

sage: G = Graph(immutable=True)
sage: G.add_edge([0,1])
---------------------------------------------------------------------------
NotImplementedError                       Traceback (most recent call last)
<ipython-input-20-2543f955ba93> in <module>()
----> 1 G.add_edge([Integer(0),Integer(1)])

/home/jonathan/Applications/sage/local/lib/python3.7/site-packages/sage/graphs/generic_graph.py in add_edge(self, u, v, label)
  10816                     pass
  10817
> 10818         self._backend.add_edge(u, v, label, self._directed)
  10819
  10820     def add_edges(self, edges, loops=True):

/home/jonathan/Applications/sage/local/lib/python3.7/site-packages/sage/graphs/base/graph_backends.pyx in sage.graphs.base.graph_backends.GenericGraphBackend.add_edge (build/cythonized/sage/graphs/base/graph_backends.c:1694)()
    100             NotImplementedError
    101          """
--> 102         raise NotImplementedError()
    103
    104     def add_edges(self, edges, directed):

NotImplementedError:

sage: G = DiGraph([[0,1]], immutable=True)
sage: G.reverse_edges(G.edges())
---------------------------------------------------------------------------
NotImplementedError                       Traceback (most recent call last)
<ipython-input-22-b7007f357194> in <module>()
----> 1 G.reverse_edges(G.edges())

/home/jonathan/Applications/sage/local/lib/python3.7/site-packages/sage/graphs/digraph.py in reverse_edges(self, edges, inplace, multiedges)
   2074         tempG = self if inplace else copy(self)
   2075         for e in edges:
-> 2076             tempG.reverse_edge(e,inplace=True,multiedges=multiedges)
   2077         if not inplace:
   2078             return tempG

/home/jonathan/Applications/sage/local/lib/python3.7/site-packages/sage/graphs/digraph.py in reverse_edge(self, u, v, label, inplace, multiedges)
   1968                                  "multiedges is True or False.")
   1969         else:
-> 1970             tempG.delete_edge(u, v, label)
   1971             tempG.add_edge(v, u, label)
   1972

/home/jonathan/Applications/sage/local/lib/python3.7/site-packages/sage/graphs/generic_graph.py in delete_edge(self, u, v, label)
  11130                     u, v = u
  11131                     label = None
> 11132         self._backend.del_edge(u, v, label, self._directed)
  11133
  11134     def delete_edges(self, edges):

/home/jonathan/Applications/sage/local/lib/python3.7/site-packages/sage/graphs/base/graph_backends.pyx in sage.graphs.base.graph_backends.GenericGraphBackend.del_edge (build/cythonized/sage/graphs/base/graph_backends.c:2273)()
    248             NotImplementedError
    249         """
--> 250         raise NotImplementedError()
    251
    252     def del_vertex(self, v):

NotImplementedError:

[Travis Scrimshaw]

Hi Jonathan,

   You are trying to mutate an immutable graph (the default is inplace=True), so it is not surprising that it is throwing an error IMO. However, the error message and type should be improved; in particular reverse_edge() should raise an error if inplace=True and the graph is immutable.

Best,

Travis

[Travis Scrimshaw]

Sorry, I think I misinterpreted the question. The solution I am proposing is correct in some sense, but the better (or just another?) thing to do would be to have the corresponding mutation methods of the immutable backends raise a more appropriate error.

Best,

Travis

[Jonathan Kliem]

The point is exactly that the error message should be more meaningful. If a user doesn't expect an immutable object, it could be tuff to figure out the problem.

For example vectors give a more meaningful message, which I would just copy for matrices:

ValueError: vector is immutable; please change a copy instead (use copy())

As I'm not into graphs very much, I wanted to have some feedback before I just go along and open a ticket for this.

[Travis Scrimshaw]

I always think more meaningful messages are good (with the correct type of error too). So +1 on opening a ticket.

Best,

Travis

[Jonathan Kliem]

Ok. I opened a ticket for it.

https://trac.sagemath.org/ticket/28874

--
You received this message because you are subscribed to a topic in the Google Groups "sage-devel" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/sage-devel/wOrcpr_k0CA/unsubscribe.
To unsubscribe from this group and all its topics, send an email to sage-devel+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sage-devel/95fb4784-a2f0-4a3d-a2e0-39402969964d%40googlegroups.com.