Opened 4 years ago

Closed 4 years ago

#17156 closed defect (fixed)

Creating a graph from a immutable digraph raises an error

Reported by: tscrim Owned by: tscrim
Priority: major Milestone: sage-6.4
Component: graph theory Keywords: immutable graph
Cc: ncohen Merged in:
Authors: Nathann Cohen Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 24d81bb (Commits) Commit: 24d81bb6c34166acb6394935ee6d77f093c7674b
Dependencies: Stopgaps:

Description

We currently have the following:

sage: D = DiGraph([[1, 2]], immutable=True)
sage: Graph(D)
---------------------------------------------------------------------------
NotImplementedError                       Traceback (most recent call last)
...
NotImplementedError: An immutable graph does not change name

For the record, this works

sage: D = DiGraph([[1, 2]])
sage: Graph(D)
Graph on 2 vertices

I'm also wondering about the error type of (attempted) mutations of immutable (di)graphs. Some raise a ValueError, others raise a NotImplementedError. This needs to be made consistent at some point.

Change History (7)

comment:1 Changed 4 years ago by ncohen

That's because two functions have been implemented by an idiot, and this idiot is me. Fix in 30 minutes at most.

Nathann

comment:2 Changed 4 years ago by ncohen

  • Authors set to Nathann Cohen
  • Branch set to u/ncohen/17156
  • Status changed from new to needs_review

Here it is. Besides, for some reason the 'sparse' keyword was deprecated in DiGraph.to_undirected but not in Graph.to_directed, to be replaced by 'data_structure'. As the !Graph/DiGraph constructors both have the two arguments, I chose to "solve that" by removing the deprecation in DiGraph. That is the second commit.

Nathann

comment:3 Changed 4 years ago by git

  • Commit set to 24d81bb6c34166acb6394935ee6d77f093c7674b

Branch pushed to git repo; I updated commit sha1. New commits:

a928f81trac #17156: Creating a graph from a immutable digraph raises an error
24d81bbtrac #17156: Remove a deprecation warning

comment:4 Changed 4 years ago by jmantysalo

  • Cc jmantysalo removed

Tested and seems to work. However, I do not know enought Sage internals to be able to really review this. Sorry.

comment:5 Changed 4 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM. Thanks.

comment:6 in reply to: ↑ description Changed 4 years ago by jdemeyer

Replying to tscrim:

I'm also wondering about the error type of (attempted) mutations of immutable (di)graphs. Some raise a ValueError, others raise a NotImplementedError.

NotImplementedError is certainly not right, it seems to imply that it could get implemented someday. I also find ValueError dubious, since there is not really a "value" which is wrong.

I would actually vote for TypeError, which is what you get when you an operation which is not supported on some type, similar to

>>> "aaa"/3
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unsupported operand type(s) for /: 'str' and 'int'

comment:7 Changed 4 years ago by vbraun

  • Branch changed from u/ncohen/17156 to 24d81bb6c34166acb6394935ee6d77f093c7674b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.