Opened 6 years ago

Closed 6 years ago

#17086 closed defect (fixed)

GenericGraph's documentation, __eq__, and __hash__ out of sync

Reported by: emassop Owned by:
Priority: major Milestone: sage-6.4
Component: graph theory Keywords:
Cc: SimonKing Merged in:
Authors: Erik Massop Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: dd544a1 (Commits) Commit: dd544a182d5948539f578e71776d6b5d2097d409
Dependencies: #17092 Stopgaps:

Description (last modified by emassop)

  1. The documentation of GenericGraph.__eq__ contains "For equality, ..., output the same vertex list (in order), ...". However, since b59285e6 (Trac #14806: Immutable graph backend), the order of the vertices is no longer checked. So either that check should be reintroduced, or the documentation should be updated.

The list returned by .vertices() is sorted, so that checking the order only affects the result in rare cases. Hence I think updating the documentation should be okay, even while it is technically backwards incompatible.

  1. While GenericGraph.__eq__ no longer takes the ordering of vertices into account, GenericGraph.__hash__ still does. Consequently graphs that compare equal can have different hashes.
  1. For graphs that are not weighted, GenericGraph.__eq__ ignores the edge labels, while GenericGraph.__hash__ does not.

Item 3 is most easily shown:

sage: G1 = Graph({0: {1: 'edge label A'}}, immutable=True)
sage: G2 = Graph({0: {1: 'edge label B'}}, immutable=True)
sage: G1 == G2
True
sage: G1.__hash__() == G2.__hash__()
False

For items 2 and 3, here is some contrived but deterministic code showing that the order is not checked in .__eq__, while it is in .__hash__:

import functools
from sage.graphs.graph import Graph

@functools.total_ordering
class C:
    order = ((0,0), (0,1), (1,1), (1,0))
    # Hasse diagram:
    #   0,0 < 0,1
    #    ^     ^
    #   1,0 > 1,1

    def __init__(self, x):
        assert x in self.order
        self.x = x
    def __repr__(self):
        return 'C(%r)' % (self.x,)

    # ordering depends on full self.x
    def __lt__(self, other):
        return self.order.index(self.x) < self.order.index(other.x)

    # equality depends only on the second coordinate.
    def __eq__(self, other):
        return self.x[1] == other.x[1]
    def __hash__(self):
        return hash(self.x[1])


G1 = Graph({C((0, 0)): [], C((0, 1)): []}, immutable=True)
G2 = Graph({C((1, 0)): [], C((1, 1)): []}, immutable=True)
print (G1.vertices(), G2.vertices())
print G1 == G2
print hash(G1) == hash(G2)
sage: %run /tmp/evil2.py
([C((0, 0)), C((0, 1))], [C((1, 1)), C((1, 0))])
True
False

The trick in the code above is to obtain lists [x1, y1] and [x2, y2] that compare equal but sort differently. In this case we use [C((0,0)), C((0,1))] and [C((1,0)), C((1,1))].

Such lists also occur naturally, as the code below shows. Here the inequality comparison falls back to comparison of ids, so we just generate objects until the ids are in the unlucky order.

from random import randint
from sage.graphs.graph import Graph

class Foo:
    def __init__(self, b):
        self.b = b
    def __eq__(self, other):
        return self.b == other.b
    def __hash__(self):
        return hash(self.b)
    def __repr__(self):
        return "<foo(%r) at %r>" % (self.b, id(self))

def gen(f):
    print "* Trying to obtain res = [[f(0),f(1)], [f(0),f(1)]] with\n" \
          "  (not res[0][0] < res[0][1]) and (res[1][0] < res[1][1])."
    candidates = [[],[]]
    res = [None, None]
    while res[False] is None or res[True] is None:
        bit = randint(0,1)
        candidates[bit].append( f(bit) )
        for x in candidates[0]:
            for y in candidates[1]:
                res[bool(x < y)] = [x,y]
    print "* Succeeded after %i random picks." \
           % (len(candidates[0]) + len(candidates[1]),)
    print ("* res[0] = %r\n  res[1] = %r") % (res[False], res[True])
    return res

def doit(f):
    print
    print "* f = %r" % (f,)
    res = gen(f)
    print "* For i=0 and i=1 creating graph G[i] with vertices " \
          "res[i] and no edges"
    G = [Graph(dict((x, []) for x in res[bit]), immutable = True)
         for bit in range(2)]
    print "* G[0] = %r\n  G[1] = %r" % (G[0], G[1])
    print "* G[0].vertices() = %r\n  G[1].vertices() = %r" % \
            (G[0].vertices(), G[1].vertices())
    print "*         res[0] == res[1]         : %r" % \
            (res[0] == res[1],)
    print "* sorted(res[0]) == sorted(res[1]) : %r" % \
            (sorted(res[0]) == sorted(res[1]),)
    print "*           G[0] == G[1]           : %r" % (G[0] == G[1],)
    print "*     hash(G[0]) == hash(G[1])     : %r" % \
            (hash(G[0]) == hash(G[1]),)
    print
sage: %run /tmp/evil.py
sage: doit(Foo)

* f = <class __main__.Foo at 0xad1caadc>
* Trying to obtain res = [[f(0),f(1)], [f(0),f(1)]] with
  (not res[0][0] < res[0][1]) and (res[1][0] < res[1][1]).
* Succeeded after 5 random picks.
* res[0] = [<foo(0) at 2904422892L>, <foo(1) at 2904420844L>]
  res[1] = [<foo(0) at 2904422892L>, <foo(1) at 2904424140L>]
* For i=0 and i=1 creating graph G[i] with vertices res[i] and no edges
* G[0] = Graph on 2 vertices
  G[1] = Graph on 2 vertices
* G[0].vertices() = [<foo(1) at 2904420844L>, <foo(0) at 2904422892L>]
  G[1].vertices() = [<foo(0) at 2904422892L>, <foo(1) at 2904424140L>]
*         res[0] == res[1]         : True
* sorted(res[0]) == sorted(res[1]) : False
*           G[0] == G[1]           : True
*     hash(G[0]) == hash(G[1])     : False

The custom class Foo above is not necessary:

sage: doit(lambda x: sage.graphs.graph.Graph(x, immutable=True))

* f = <function <lambda> at 0xad1d9e9c>
* Trying to obtain res = [[f(0),f(1)], [f(0),f(1)]] with
  (not res[0][0] < res[0][1]) and (res[1][0] < res[1][1]).
* Succeeded after 3 random picks.
* res[0] = [Graph on 0 vertices, Graph on 1 vertex]
  res[1] = [Graph on 0 vertices, Graph on 1 vertex]
* For i=0 and i=1 creating graph G[i] with vertices res[i] and no edges
* G[0] = Graph on 2 vertices
  G[1] = Graph on 2 vertices
* G[0].vertices() = [Graph on 1 vertex, Graph on 0 vertices]
  G[1].vertices() = [Graph on 0 vertices, Graph on 1 vertex]
*         res[0] == res[1]         : True
* sorted(res[0]) == sorted(res[1]) : False
*           G[0] == G[1]           : True
*     hash(G[0]) == hash(G[1])     : False

Change History (30)

comment:1 Changed 6 years ago by emassop

  • Branch set to u/emassop/graph_hash

comment:2 Changed 6 years ago by ncohen

  • Cc SimonKing added
  • Commit set to 82b245ee0b092a7a994074e27cc47686d43551c9

New commits:

82b245eTrac #17086: GenericGraph's doc, __eq__, and __hash__ out of sync

comment:3 follow-up: Changed 6 years ago by ncohen

Hello !

Thank you for this 'Counter' thing, I did not know that it existed ^^;

1) About self.edge_iterator(labels = self._weighted):

You can have graphs with edge labels for which _weighted is not True

sage: Graph([(1,2,"Hey")])._weighted
False
sage: Graph([(1,2,"Hey")],immutable=True)._weighted
False

I do not think that this 'weighted' notion makes a lot of sense anyway, perhaps we should remove it. Either way it is probably best to set 'labels' to True anytime.

2) Perhaps I do not understand what your tests do, but even when the graph is immutable the list of vertices returned by .vertices() should be sorted. Do you have a counterexample to that ?

Indeed, we cannot assume that sorting a list produces a unique sorted list (because the order is not always a total order) but that list, however, should be sorted.

This, because even when the immutable graph is created, the vertices are labelled using the order returned by .vertices(). So I hope that you found no bad behaviour there.

But indeed, wrapping it with a frozenset if the right thing to do.

3) Technically, you do not need to add ._is_weighted in the hash function, as graphs with different values for ._is_weighted are nonequal anyway. But that does not hurt either.

4) O_O

sage: G1 = Graph({0: {1: 'edge label A'}})                
sage: G2 = Graph({0: {1: 'edge label B'}})                
sage: G1 == G2                            
True

This is *REALLY* bad ! Graphs with different labels should be non-equal !!! O_O;;;;;

But it seems that it has been like that forever... Gosh that's bad O_o

Hmmmm.. Now I understand why you added this self._weighted in the hash function :-/

Nathann

comment:4 in reply to: ↑ 3 ; follow-up: Changed 6 years ago by emassop

  • Description modified (diff)

Replying to ncohen:

Thank you for this 'Counter' thing, I did not know that it existed ^^;

It's shiny new Python 2.7 material :).


1) About self.edge_iterator(labels = self._weighted):

You can have graphs with edge labels for which _weighted is not True

sage: Graph([(1,2,"Hey")])._weighted
False
sage: Graph([(1,2,"Hey")],immutable=True)._weighted
False

I do not think that this 'weighted' notion makes a lot of sense anyway, perhaps we should remove it. Either way it is probably best to set 'labels' to True anytime.

Indeed you can set edge labels on unweighted graphs, but the documented behaviour is that they are ignored in __eq__: "Note that graphs must be considered weighted, or Sage will not pay attention to edge label data in equality testing". Whether edge labels should be checked or not, is a different question, that I would like to keep outside the scope of this ticket. Either way, when __eq__ is true, the hashes have to be the same. So if __eq__ ignores labels, then so should __hash__. I really see no point in ignoring edge labels in __hash__ when they are not ignored in __eq__. I think that would just makes the hash weaker.


2) Perhaps I do not understand what your tests do, but even when the graph is immutable the list of vertices returned by .vertices() should be sorted. Do you have a counterexample to that ?

Indeed, we cannot assume that sorting a list produces a unique sorted list (because the order is not always a total order) but that list, however, should be sorted.

This, because even when the immutable graph is created, the vertices are labelled using the order returned by .vertices(). So I hope that you found no bad behaviour there.

No, I do not have a counter example. I might look into it, or not. Either way, I would like to restrict this ticket to the totally ordered case where the axioms for ordering use is and not ==.

When an is-based ordering does not respect ==, such as Python's default ordering using id, you can have a1==a2 and b1==b2 and a1<b1 and a2>b2 at the same time. Within a single graph this cannot happen as there a1==a2 implies a1 is a2 (assuming that the given vertex labels are ==-distinct). However, it is possible to construct a pair of graphs using ==-equal vertex lists that nonetheless have distinct .vertices(). This is what happens above.

In the example using class C, I have explicitly constructed a comparison that gives such evil results. (Presently I have modified it to obey the axioms of an is-based ordering.) In the example using class Foo, I show that the problem also occurs with Python's default ordering. Since Python's default ordering depends on id over which you have no control, you need a bit of luck. The example using lambda x: ..., shows that the behaviour of class Foo occurs in the wild.

But indeed, wrapping it with a frozenset if the right thing to do.

:)


3) Technically, you do not need to add ._is_weighted in the hash function, as graphs with different values for ._is_weighted are nonequal anyway. But that does not hurt either.

In hash(..., self._is_weighted, ...) I indeed included it redundantly.


4) O_O

sage: G1 = Graph({0: {1: 'edge label A'}})                
sage: G2 = Graph({0: {1: 'edge label B'}})                
sage: G1 == G2                            
True

This is *REALLY* bad ! Graphs with different labels should be non-equal !!! O_O;;;;;

But it seems that it has been like that forever... Gosh that's bad O_o

According to the documentation the labels are ignored. Let's defer what is right(tm) to a different ticket.

Hmmmm.. Now I understand why you added this self._weighted in the hash function :-/

:)

comment:5 Changed 6 years ago by git

  • Commit changed from 82b245ee0b092a7a994074e27cc47686d43551c9 to a207b85b076036104911b312aa5639a0b135a2e0

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

06a3a12fix typo
a207b85Use more illustrative test

comment:6 in reply to: ↑ 4 ; follow-up: Changed 6 years ago by ncohen

Yoooooooo !

It's shiny new Python 2.7 material :).

Niiiiiiiiiice :-P

Indeed you can set edge labels on unweighted graphs, but the documented behaviour is that they are ignored in __eq__: "Note that graphs must be considered weighted, or Sage will not pay attention to edge label data in equality testing".

Weird. I had no idea and expected something different. Well. Nothing wrong if it works as claimed, then :-P

Whether edge labels should be checked or not, is a different question, that I would like to keep outside the scope of this ticket.

Right right. I just did not expect that and I thought it was a bug. I don't mind it if it is not.

Either way, when __eq__ is true, the hashes have to be the same. So if __eq__ ignores labels, then so should __hash__. I really see no point in ignoring edge labels in __hash__ when they are not ignored in __eq__. I think that would just makes the hash weaker.

Of course, what you do makes sense in that case.

No, I do not have a counter example. I might look into it, or not. Either way, I would like to restrict this ticket to the totally ordered case where the axioms for ordering use is and not ==.

When an is-based ordering does not respect ==, such as Python's default ordering using id, you can have a1==a2 and b1==b2 and a1<b1 and a2>b2 at the same time. Within a single graph this cannot happen as there a1==a2 implies a1 is a2 (assuming that the given vertex labels are ==-distinct). However, it is possible to construct a pair of graphs using ==-equal vertex lists that nonetheless have distinct .vertices(). This is what happens above.

Okay, it took me a while to understand but it seems that to you a 'is-based' ordering when comparison does not yield a total order, and is not necessarily agreeing with '=='. Why not. It has to work in this case too, anyway.

Despite that, I call a list L 'sorted' when applying 'sorted' to L does not change L. Even if the ordering only makes sense to Python. And lists returned by .vertices() should always be sorted in this way. I do not think that it is very bad if it does not hold, but that is how it is meant to work.

Since Python's default ordering depends on id over which you have no control, you need a bit of luck.

I would not have complained even if you had build a class for which <= returns a random value :-P

In hash(..., self._is_weighted, ...) I indeed included it redundantly.

You were right.

According to the documentation the labels are ignored. Let's defer what is right(tm) to a different ticket.

Well, I do not think it needs a different ticket. I was surprised, but that's as bad as it got. If it is documented I don't see anything wrong with it. Especially since is_isomorphic also ignores edges by default.

The only problem is this 'weighted' keyword. It does not have anything to do with weights, so the keyword is ill-chosen. This can be fixed in another ticket, but there is nothing really urgent to solve either.

Nathann


New commits:

06a3a12fix typo
a207b85Use more illustrative test

comment:7 Changed 6 years ago by ncohen

Hello again !

I see nothing wrong with your code, but I fear that I may be scared of your big doctest in a couple of years if I am still around.

Could you make it more explicit, like

Equality and hash do not depend on ordering of vertices (i.e. G==H can be true even when G.vertices()==H.vertices() is False)

Thanks,

Nathann

comment:8 in reply to: ↑ 6 ; follow-up: Changed 6 years ago by emassop

Replying to ncohen:

No, I do not have a counter example. I might look into it, or not. Either way, I would like to restrict this ticket to the totally ordered case where the axioms for ordering use is and not ==.

When an is-based ordering does not respect ==, such as Python's default ordering using id, you can have a1==a2 and b1==b2 and a1<b1 and a2>b2 at the same time. Within a single graph this cannot happen as there a1==a2 implies a1 is a2 (assuming that the given vertex labels are ==-distinct). However, it is possible to construct a pair of graphs using ==-equal vertex lists that nonetheless have distinct .vertices(). This is what happens above.

Okay, it took me a while to understand but it seems that to you a 'is-based' ordering when comparison does not yield a total order, and is not necessarily agreeing with '=='. Why not. It has to work in this case too, anyway.

I can't understand "to you ... with '=='" here. The problem can occur when an is-based ordering disagrees with ==. This can still happen when it is total.

To be precise, the problem occurs when

  • there are x, y, and z with x < y, y < z and x == z.

This does not contradict the following axioms of an is-based total ordering:

  • x < y and y < z implies x < z for all x, y, z
  • for all x and y exactly one of x < y, x is y, and y < x holds.

Despite that, I call a list L 'sorted' when applying 'sorted' to L does not change L. Even if the ordering only makes sense to Python. And lists returned by .vertices() should always be sorted in this way. I do not think that it is very bad if it does not hold, but that is how it is meant to work.

If sorted only uses < and not ==, and if < is an is-based total order, then sorted(sorted(L)) == sorted(L) is indeed true. So someone should maybe check the implementation of sorted to see what comparisons are used.

According to the documentation the labels are ignored. Let's defer what is right(tm) to a different ticket.

Well, I do not think it needs a different ticket. I was surprised, but that's as bad as it got. If it is documented I don't see anything wrong with it. Especially since is_isomorphic also ignores edges by default.

Okay... your punctuation was a bit strange then: "This is *REALLY* bad ! Graphs with different labels should be non-equal !!! O_O;;;;;"

The only problem is this 'weighted' keyword. It does not have anything to do with weights, so the keyword is ill-chosen. This can be fixed in another ticket, but there is nothing really urgent to solve either.

Agreed.

comment:9 in reply to: ↑ 8 Changed 6 years ago by ncohen

I can't understand "to you ... with '=='" here. The problem can occur when an is-based ordering disagrees with ==. This can still happen when it is total.

Okayokay right. So it can happen even when the ordering is total, when == does not agree with <. Or actually, it happens when < is not an order at all.

If sorted only uses < and not ==, and if < is an is-based total order, then sorted(sorted(L)) == sorted(L) is indeed true. So someone should maybe check the implementation of sorted to see what comparisons are used.

I expect that it only uses < or <=.

Well, I do not think it needs a different ticket. I was surprised, but that's as bad as it got. If it is documented I don't see anything wrong with it. Especially since is_isomorphic also ignores edges by default.

Okay... your punctuation was a bit strange then: "This is *REALLY* bad ! Graphs with different labels should be non-equal !!! O_O;;;;;"

Well, I *did* freak out. But if it works as intended, no problem :-P

Nathann

comment:10 Changed 6 years ago by git

  • Commit changed from a207b85b076036104911b312aa5639a0b135a2e0 to 5a2a6411c32286a29e18781245c353e3f78e1cff

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

5a2a641Clarify comment as suggested by Nathann Cohen

comment:11 Changed 6 years ago by emassop

  • Authors set to Erik Massop
  • Status changed from new to needs_review

comment:12 Changed 6 years ago by ncohen

  • Reviewers set to Nathann Cohen
  • Status changed from needs_review to positive_review

Thank you very much !

Nathann

comment:13 Changed 6 years ago by emassop

At http://build.sagedev.org/trac/builders/trac_builder/builds/1112, there are these failures:

----------------------------------------------------------------------
sage -t src/sage/crypto/mq/sr.py  # 5 doctests failed
sage -t src/sage/modular/modform/numerical.py  # 2 doctests failed
sage -t src/sage/quivers/path_semigroup.py  # 3 doctests failed
sage -t src/sage/rings/polynomial/multi_polynomial_sequence.py  # 9 doctests failed
sage -t src/sage/rings/polynomial/pbori.pyx  # Killed due to segmentation fault
sage -t src/sage/rings/polynomial/polynomial_element.pyx  # 2 doctests failed
sage -t src/sage/rings/real_double.pyx  # 2 doctests failed
----------------------------------------------------------------------

Of these

sage -t src/sage/modular/modform/numerical.py  # 2 doctests failed
sage -t src/sage/rings/polynomial/polynomial_element.pyx  # 2 doctests failed
sage -t src/sage/rings/real_double.pyx  # 2 doctests failed

seem to be numerical noise that I can't reproduce. Moreover

sage -t src/sage/crypto/mq/sr.py  # 5 doctests failed
sage -t src/sage/rings/polynomial/multi_polynomial_sequence.py  # 9 doctests failed
sage -t src/sage/rings/polynomial/pbori.pyx  # Killed due to segmentation fault

are pbori segfaults, that I also get after reverting my patches for this ticket. The remaining failed doctest is

sage -t src/sage/quivers/path_semigroup.py  # 3 doctests failed

which is indeed caused by my patches here.

Here is the output:

sage -t src/sage/quivers/path_semigroup.py
**********************************************************************
File "src/sage/quivers/path_semigroup.py", line 179, in sage.quivers.path_semigroup.PathSemigroup._coerce_map_from_
Failed example:
    P1.has_coerce_map_from(P3)
Expected:
    False
Got:
    True
**********************************************************************
File "src/sage/quivers/path_semigroup.py", line 195, in sage.quivers.path_semigroup.PathSemigroup._coerce_map_from_
Failed example:
    c3 in P1   # indirect doctest
Expected:
    False
Got:
    True
**********************************************************************
File "src/sage/quivers/path_semigroup.py", line 197, in sage.quivers.path_semigroup.PathSemigroup._coerce_map_from_
Failed example:
    d*c3
Expected:
    Traceback (most recent call last):
    ...
    TypeError: unsupported operand parent(s) for '*':
     'Partial semigroup formed by the directed paths of Multi-digraph on 3 vertices'
     and 'Partial semigroup formed by the directed paths of Multi-digraph on 3 vertices'
Got:
    d*c
**********************************************************************
1 item had failures:
   3 of  18 in sage.quivers.path_semigroup.PathSemigroup._coerce_map_from_
    [151 tests, 3 failures, 0.83 s]

and the failing code:

    def _coerce_map_from_(self, other):
        """
        A coercion from `A` to `B` exists if the underlying quiver
        of `A` is a sub-quiver of the underlying quiver of `B` (preserving
        names).

        EXAMPLES::

            sage: Q1 = DiGraph({1:{2:['a','b'], 3:['c']}, 3:{1:['d']}})
            sage: Q2 = DiGraph({1:{2:['a'], 3:['c']}})
            sage: Q3 = DiGraph({1:{2:['a','x'], 3:['c']}, 3:{1:['d']}})
            sage: P1 = Q1.path_semigroup()
            sage: P2 = Q2.path_semigroup()
            sage: P3 = Q3.path_semigroup()
            sage: P1.has_coerce_map_from(P2)   # indirect doctest
            True
            sage: P1.has_coerce_map_from(P3)
            False
            sage: d = P1([(3,1,'d')]); d
            d
            sage: c = P2([(1,3,'c')]); c
            c
            sage: c.parent() is P1
            False
            sage: c in P1    # indirect doctest
            True
            sage: d*c        # indirect doctest
            d*c
            sage: (d*c).parent() is P1
            True
            sage: c3 = P3([(1,3,'c')]); c3
            c
            sage: c3 in P1   # indirect doctest
            False
            sage: d*c3
            Traceback (most recent call last):
            ...
            TypeError: unsupported operand parent(s) for '*':
             'Partial semigroup formed by the directed paths of Multi-digraph on 3 vertices'
             and 'Partial semigroup formed by the directed paths of Multi-digraph on 3 vertices'
        """

The problem seems to be that Q1 == Q3 is true while it should be false as the labels ought to be checked. This in turn makes P1 == P3 true. This didn't register before as the hashes were different, so that the unique representations P1 and P3 also became different.

Running git grep path_semigroup, I see that the digraphs on which path_semigroup is called are nowhere 'weighted'. It therefore seems that the people working on quivers assume that edge labels are checked... This is now bug #17092.

comment:14 Changed 6 years ago by emassop

  • Dependencies set to #17092

comment:15 Changed 6 years ago by vbraun

  • Status changed from positive_review to needs_work

comment:16 Changed 6 years ago by vbraun

  • Status changed from needs_work to positive_review

Got these on the buildbot, too.

comment:17 Changed 6 years ago by ncohen

  • Status changed from positive_review to needs_work

comment:18 follow-up: Changed 6 years ago by ncohen

  • Status changed from needs_work to needs_review

(does not pass tests in sage/quivers/path_semigroup.py)

comment:19 in reply to: ↑ 18 ; follow-up: Changed 6 years ago by SimonKing

Replying to ncohen:

(does not pass tests in sage/quivers/path_semigroup.py)

With #17092 applied?

comment:20 in reply to: ↑ 19 Changed 6 years ago by ncohen

With #17092 applied?

I do not know, I just tested it by itself. Anyway Volker does not want to see a patch in positive_review before all its dependencies. And I wonder if it is the good fix, by the way, for I really expected that two graphs with different labels would be returned as nonequal, regardless of this misnamed weighted parameter.

Nathann

comment:21 follow-up: Changed 6 years ago by vbraun

Its fine to have this as positive review before the dependency is finished, assuming that tests will pass once the dependency is finished.

If you have any doubts then you should sort that out, of course.

comment:22 in reply to: ↑ 21 Changed 6 years ago by ncohen

Its fine to have this as positive review before the dependency is finished, assuming that tests will pass once the dependency is finished.

You set a designs ticket of mine to "needs_work" for that very reason not so long ago.

comment:23 follow-up: Changed 6 years ago by vbraun

Which ticket? In general I think it should be avoided to have positively-reviewed tickets hang around for a long time while waiting for a dependency. But I have improved the release management scripts a few months ago to handle this case more easily.

Last edited 6 years ago by vbraun (previous) (diff)

comment:24 in reply to: ↑ 23 Changed 6 years ago by ncohen

Which ticket? In general I think it should be avoided to have positively-reviewed tickets hang around for a long time while waiting for a dependency. But I have improved the release management scripts a few months ago to handle this case more easily.

Okay.

comment:25 Changed 6 years ago by git

  • Commit changed from 5a2a6411c32286a29e18781245c353e3f78e1cff to 25fbd62a2c27afa75d798300c123a1b885acf89b

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

25fbd62More explicit about edge labels not being checked

comment:26 Changed 6 years ago by emassop

IMHO __eq__ should appear in the html/pdf documentation somewhere, with all this confusion about what is checked. How do I do that? and where should it appear?

comment:27 Changed 6 years ago by vbraun

You need to add .. automethod: __eq__ to the class docstringc.

comment:28 Changed 6 years ago by git

  • Commit changed from 25fbd62a2c27afa75d798300c123a1b885acf89b to 13a43f9870d62e48cc752ffd48f9dbb92faee4d8

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

13a43f9Add __eq__ to documentation

comment:29 Changed 6 years ago by ncohen

  • Branch changed from u/emassop/graph_hash to public/17086
  • Commit changed from 13a43f9870d62e48cc752ffd48f9dbb92faee4d8 to dd544a182d5948539f578e71776d6b5d2097d409
  • Status changed from needs_review to positive_review

I am running the doctests again just in case, but the tests pass in quivers/ when #17092 is applied.

I change this ticket's branch to include a merge commit with #17092.

Thanks !

Nathann


New commits:

cddc969Force weighted graph for path_semigroup
628b03cMimic argument ordering of constructor
ddf2cb7Simply pass weighted to constructor
5607441Fix quotes in documentation
dd544a1trac #17086: Merged with #17092

comment:30 Changed 6 years ago by vbraun

  • Branch changed from public/17086 to dd544a182d5948539f578e71776d6b5d2097d409
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.