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
comment:2 Changed 4 years ago by
- 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
- Commit set to 24d81bb6c34166acb6394935ee6d77f093c7674b
comment:4 Changed 4 years ago by
- 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
- 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
Replying to tscrim:
I'm also wondering about the error type of (attempted) mutations of immutable (di)graphs. Some raise a
ValueError
, others raise aNotImplementedError
.
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
- Branch changed from u/ncohen/17156 to 24d81bb6c34166acb6394935ee6d77f093c7674b
- Resolution set to fixed
- Status changed from positive_review to closed
That's because two functions have been implemented by an idiot, and this idiot is me. Fix in 30 minutes at most.
Nathann