Ticket #5003 (closed defect: fixed)
[with patch, positive review] equality testing in graphs should check "weighted" property
| Reported by: | rlm | Owned by: | rlm |
|---|---|---|---|
| Priority: | major | Milestone: | sage-3.3 |
| Component: | graph theory | Keywords: | |
| Cc: | Work issues: | ||
| Report Upstream: | Reviewers: | ||
| Authors: | Merged in: | ||
| Dependencies: | Stopgaps: |
Attachments
Change History
comment:1 follow-up: ↓ 2 Changed 4 years ago by shumow
- Summary changed from [with patch, needs review] equality testing in graphs should check "weighted" property to [with patch, needs work] equality testing in graphs should check "weighted" property
Hey, I ran into some doctest failures w/ your new change.
Specifically, around line 839 (in the docstring for weighted_ajacency_matrix(...)):
EXAMPLES:
sage: G = Graph(sparse=True)
sage: G.add_edges([(0,1,1),(1,2,2),(0,2,3),(0,3,4)])
sage: M = G.weighted_adjacency_matrix(); M
[0 1 3 4]
[1 0 2 0]
[3 2 0 0]
[4 0 0 0]
sage: H = Graph(data=M, format='weighted_adjacency_matrix', sparse=True)
sage: H == G
True
This fails. Specifically, G.weighted() returns false (which seems like its own bug.)
And Also, the example starting at line 1180 (in the docstring for weighted(...):
EXAMPLE:
Here we have two graphs with different labels, but weighted is False
for both, so we just check for the presence of edges:
sage: G = Graph({0:{1:'a'}}, implementation='networkx')
sage: H = Graph({0:{1:'b'}}, implementation='networkx')
sage: G == H
True
Now one is weighted and the other is not, so the comparison is done as
if neither is weighted:
sage: G.weighted(True)
sage: H.weighted()
False
sage: G == H
True
Fails. Because of the change.
The first of these issues, is a bug and should be fixed IMHO. The second issue is more subtle and disturbing. Particularly because it indicates that a valid example used to work, you will be breaking compatibility with code that works this way, and you should think about what the previous assumptions were, and if you can work around them with a fix.
comment:2 in reply to: ↑ 1 Changed 4 years ago by rlm
- Summary changed from [with patch, needs work] equality testing in graphs should check "weighted" property to [with patch, needs review] equality testing in graphs should check "weighted" property
Replying to shumow:
This fails. Specifically, G.weighted() returns false (which seems like its own bug.)
Not so much a bug, as a typo in the doctest. If you don't say G is weighted, then just adding edges with weights shouldn't change that. In fact, that's the point of the other doctest.
And Also, the example starting at line 1180 (in the docstring for weighted(...):
...
Fails. Because of the change.
The second issue is more subtle and disturbing. Particularly because it indicates that a valid example used to work, you will be breaking compatibility with code that works this way, and you should think about what the previous assumptions were, and if you can work around them with a fix.
Well, it's more like we're updating things to actually do it correctly. Before, weighted wasn't a property of graphs, and that test was kind of a warning about that. I don't know of any code that would be affected by this, but I think this is the right way to do things.

