I always think more meaningful messages are good (with the correct type of error too). So +1 on opening a ticket.
Best, Travis On Thursday, December 12, 2019 at 3:54:13 PM UTC+10, Jonathan Kliem wrote: > > 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. > > Am Donnerstag, 12. Dezember 2019 01:49:57 UTC+1 schrieb 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 >> >> >> On Thursday, December 12, 2019 at 10:47:26 AM UTC+10, Travis Scrimshaw >> wrote: >>> >>> 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 >>> >>> >>> On Wednesday, December 11, 2019 at 9:58:05 PM UTC+10, Jonathan Kliem >>> wrote: >>>> >>>> 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 >>>> <https://trac.sagemath.org/ticket/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: >>>> >>>> >>>> -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/sage-devel/95fb4784-a2f0-4a3d-a2e0-39402969964d%40googlegroups.com.
