Ticket #5003 (closed defect: fixed)

Opened 4 years ago

Last modified 4 years ago

[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

trac-5003_weighted_eq_graphs.patch Download (1.4 KB) - added by rlm 4 years ago.
trac-5003-followup.patch Download (1.3 KB) - added by rlm 4 years ago.
Apply this patch second.

Change History

Changed 4 years ago by rlm

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.

Changed 4 years ago by rlm

Apply this patch second.

comment:3 Changed 4 years ago by mhansen

  • Summary changed from [with patch, needs review] equality testing in graphs should check "weighted" property to [with patch, positive review] equality testing in graphs should check "weighted" property

Looks good to me.

comment:4 Changed 4 years ago by mabshoff

  • Status changed from new to closed
  • Resolution set to fixed

Merged in Sage 3.3.alpha2.

Cheers,

Michael

Note: See TracTickets for help on using tickets.