pydrake symbolic: Disable Formula.__nonzero__

[EricCousineau-TRI]

Closes #8491

Note that this doesn't really help out with #8315 -- in fact, it may worsen the behavior, as NumPy seems to have odd behavior: If the elements are equal but __nonzero__ is disabled, then NumPy will return an array of Trues (possibly based on the identity of the arrays themselves?). If the elements are unequal, then NumPy will return a scalar False.

This does not address conversion for bool in the C++ code, as I would prefer to focus on the Python portion of things to better facilitate PR #8452.

---

This change is 

[EricCousineau-TRI]

+@soonho-tri for feature review, please.

---

Review status: 0 of 6 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[soonho-tri]

Review status: 0 of 6 files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

bindings/pydrake/util/pure_hash_dict.py, line 19 at r1 (raw file):

    def __eq__(self, other):
        return hash(self._value) == hash(other)

For now, we have EqualTo : T * T -> bool methods for Polynomial and Formula Python classes and I can add them to Expression and Variable Python classes. Then, we can use this to check the equality in addition to checking hash values. What do you think?

---

Comments from Reviewable

[soonho-tri]

Review status: 0 of 6 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

bindings/pydrake/util/pure_hash_dict.py, line 19 at r1 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

For now, we have EqualTo : T * T -> bool methods for Polynomial and Formula Python classes and I can add them to Expression and Variable Python classes. Then, we can use this to check the equality in addition to checking hash values. What do you think?

FYI/FWIW, #8510 is merged.

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 0 of 6 files reviewed at latest revision, 1 unresolved discussion.

---

bindings/pydrake/util/pure_hash_dict.py, line 19 at r1 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

FYI/FWIW, #8510 is merged.

Done.

---

Comments from Reviewable

[soonho-tri]

Checkpoint with a few questions/BTWs.

---

Reviewed 5 of 8 files at r2.
Review status: 4 of 6 files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

bindings/pydrake/symbolic_py.cc, line 331 at r2 (raw file):

      .def_static("True", &Formula::True)
      .def_static("False", &Formula::False)
      .def("__nonzero__", [](const Formula&) {

Do we need to disable __nonzero__ for other classes that we expect users to use EqualToDict?

---

bindings/pydrake/util/containers.py, line 75 at r2 (raw file):

By default, equality is based on type and hash.

What's the motivation to keep it (or make it as a default)?

---

bindings/pydrake/util/containers.py, line 95 at r2 (raw file):

class EqualToDict(EqualityProxyDict):
    """Implements a dictionary where keys are compared using `lhs.EqualTo(rhs)`,
    where `lhs` and `rhs` are of compatible types.

BTW, @gizatt wants to have a dictionary whose key can be either a Variable or a vector of Variables. Do you think we can use this data structure for that? (feature requested in #8495)

---

bindings/pydrake/util/containers.py, line 99 at r2 (raw file):

    def __init__(self, *args, **kwargs):
        EqualityProxyDict.__init__(
            self, *args, eq=lambda lhs, rhs: lhs.EqualTo(rhs), **kwargs)

I guess it's safer to have type(lhs) == type(rhs) and lhs.EqualTo(rhs)?

Also, I'm wondering if it makes sense 1) first to check lhs/rhs have the same type, 2) if the type provides EqualTo then use it, 3) otherwise fall back to __eq__.

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 4 of 6 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

bindings/pydrake/symbolic_py.cc, line 331 at r2 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

Do we need to disable __nonzero__ for other classes that we expect users to use EqualToDict?

OK We only need to disable __nonzero__ on any classes which are returned as a result of equality comparison which are non-bool convertible. I believe Formula is the only class, in this case.

---

bindings/pydrake/util/containers.py, line 75 at r2 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

By default, equality is based on type and hash.

What's the motivation to keep it (or make it as a default)?

OK I prefer to build rather complex behavior like this on generic types - however, this class doesn't have to be public, so I'll go ahead and hide it.

---

bindings/pydrake/util/containers.py, line 95 at r2 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

BTW, @gizatt wants to have a dictionary whose key can be either a Variable or a vector of Variables. Do you think we can use this data structure for that? (feature requested in #8495)

OK Yes, that is what this class is meant for - any class which, when compared with something, returns Formula.

---

bindings/pydrake/util/containers.py, line 99 at r2 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

I guess it's safer to have type(lhs) == type(rhs) and lhs.EqualTo(rhs)?

Also, I'm wondering if it makes sense 1) first to check lhs/rhs have the same type, 2) if the type provides EqualTo then use it, 3) otherwise fall back to __eq__.

OK I'm not sure what we gain by checking the type: If EqualTo is implemented in such a way that where it succeeds without a type match, then we've done something wrong? (I'd say implicit conversion would be OK, but I don't believe we do any implicit conversion, aside from Variable -> Expression?)

---

Comments from Reviewable

[soonho-tri]

@drake-jenkins-bot test linux-xenial-clang-bazel-experimental-debug please.

[soonho-tri]

@drake-jenkins-bot linux-xenial-clang-bazel-experimental-debug please

[EricCousineau-TRI]

Review status: 3 of 6 files reviewed at latest revision, 3 unresolved discussions.

---

bindings/pydrake/util/containers.py, line 75 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

OK I prefer to build rather complex behavior like this on generic types - however, this class doesn't have to be public, so I'll go ahead and hide it.

Done. Realized that you were right to make the base functionality private; made that change.

---

bindings/pydrake/util/containers.py, line 99 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

OK I'm not sure what we gain by checking the type: If EqualTo is implemented in such a way that where it succeeds without a type match, then we've done something wrong? (I'd say implicit conversion would be OK, but I don't believe we do any implicit conversion, aside from Variable -> Expression?)

Done. Realize that yes, it's safe as it would avoid an exception (was not thinking about that!).

---

Comments from Reviewable

[soonho-tri]

Review status: 3 of 6 files reviewed at latest revision, all discussions resolved.

---

bindings/pydrake/test/symbolic_test.py, line 496 at r4 (raw file):

        self.assertFalse(value)
        value = (e_xv == e_xv)
        self.assertEqual(value.dtype, bool)

This line fails on macOS (+ on a Ubuntu machine as well):

==================== Test output for //bindings/pydrake:_isolated/symbolic_test:
..........E......................................................................................
======================================================================
ERROR: test_relational_operators_nonzero (symbolic_test.TestSymbolicExpression)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./bindings/pydrake/test/symbolic_test.py", line 496, in test_relational_operators_nonzero
    self.assertEqual(value.dtype, bool)
AttributeError: 'bool' object has no attribute 'dtype'

---

Comments from Reviewable

[EricCousineau-TRI]

Huh... That's odd (well, more odd than the nominal behavior).

Can I ask which version of NumPy you have on both of those OSes? (I'm assuming both are Python 2.7.12)

[soonho-tri]

FYI,

macOS
$ python -c "import numpy; print(numpy.version.full_version)"
1.14.2

Ubuntu
$ python -c "import numpy; print(numpy.version.full_version)"
1.13.3

---

Review status: 3 of 6 files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[EricCousineau-TRI]

Used a virtualenv and confirmed that this issue is NumPy version-specific. Numpy < 1.13.0 (<= 1.12.1) produces an array of booleans, while NumPy >= 1.13.0 produces a boolean.

Will codify that in the test; even though both behaviors are unwanted, I'd still like to have knowledge of what to expect.

---

Review status: 3 of 6 files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[EricCousineau-TRI]

Seems that the change may have been in v1.13.0, under the heading "np.equal, np.not_equal for object arrays ignores object identity".

[EricCousineau-TRI]

Review status: 3 of 6 files reviewed at latest revision, 1 unresolved discussion.

---

bindings/pydrake/test/symbolic_test.py, line 496 at r4 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

This line fails on macOS (+ on a Ubuntu machine as well):

==================== Test output for //bindings/pydrake:_isolated/symbolic_test:
..........E......................................................................................
======================================================================
ERROR: test_relational_operators_nonzero (symbolic_test.TestSymbolicExpression)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./bindings/pydrake/test/symbolic_test.py", line 496, in test_relational_operators_nonzero
    self.assertEqual(value.dtype, bool)
AttributeError: 'bool' object has no attribute 'dtype'

Done.

---

Comments from Reviewable

[EricCousineau-TRI]

Seems that this behavior raises a DeprecationWarning, which by default is ignored. Would've been nice if they had used NumPy-specific warnings, 'cause it's rather hard to filter out pure NumPy warnings.

I've found the two warnings particular to these errors, and have now promoted to them errors, which should now resolve usability issues #8315, at least as far as error reporting goes.

Confirmed that this works for v1.11.0 (my system), v1.13.3, and v1.14.2 on Ubuntu.

[soonho-tri]

aside a FYI.

+@jwnimmer-tri for platform-review, please.

---

Reviewed 2 of 2 files at r3, 5 of 5 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.

---

bindings/pydrake/test/symbolic_test.py, line 886 at r5 (raw file):

        p += 3
        # N.B. For whatever reason, the ordering between these two is not the
        # same. Without `ToExpression`, we get an erorr that

FYI, this is because symbolic::Polynomial uses std::unordered_map to keep the pair of a Monomial and its exponent. Can we change the string comparison in assertSame, str(lhs) == str(rhs), to something like lhs.EqualTo(rhs) (only if lhs/rhs have EqualTo?) This should make the testing more robust.

---

Comments from Reviewable

[jwnimmer-tri]

Checkpoint.

---

Reviewed 1 of 8 files at r2, 2 of 2 files at r3, 4 of 5 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

bindings/pydrake/symbolic_py.cc, line 341 at r5 (raw file):

            "Should not call `__nonzero__` on `Formula`. If you are trying to "
            "make a map with `Variable`, `Expression`, or `Polynomial` as "
            "keys, please use `EqualToDict`.");

FYI We might as well spell out the full module path for this?

---

bindings/pydrake/util/containers.py, line 7 at r5 (raw file):

class _EqualityProxyBase(object):
    # TODO(eric.cousineau): Copy input object to preserve key immutability?

I don't see why EqualToDict would do this, when regular dict doesn't? If the value type disobeys the __hash__ contract, that's a bug with the value type, not something to work around here.

---

bindings/pydrake/util/containers.py, line 18 at r5 (raw file):

    def __eq__(self, other):
        raise NotImplemented

FYI Does this deserve a string message? Or is it unreachable?

---

bindings/pydrake/util/containers.py, line 28 at r5 (raw file):

class _DictKeyWrap(dict):
    # Wraps a dictionary's key access.
    def __init__(self, tmp, key_wrap, key_unwrap):

The meaning of tmp is unclear. (I suggest giving it a better name.)

---

bindings/pydrake/util/containers.py, line 28 at r5 (raw file):

class _DictKeyWrap(dict):
    # Wraps a dictionary's key access.
    def __init__(self, tmp, key_wrap, key_unwrap):

The meaning of key_wrap and key_unwrap are unclear. I guess they are both supposed to quack like single-argument functors?

---

bindings/pydrake/util/deprecation.py, line 143 at r5 (raw file):

        return
    installed_numpy_warning_filters = True
    # Warnings specific to comparison with `dtype=object`.

I think this comment needs to be about 10x larger, explaining what is going on, why we choose to filter it, whether certain numpy versions are affected, etc. -- basically everything you've learned about these deprecation things should be dumped into the comment here. As it stands, I have no way to reason about the justification for this code, nor any idea how to explore or test it.

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 4 of 8 files reviewed at latest revision, 7 unresolved discussions.

---

bindings/pydrake/symbolic_py.cc, line 341 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI We might as well spell out the full module path for this?

Done.

---

bindings/pydrake/test/symbolic_test.py, line 886 at r5 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

FYI, this is because symbolic::Polynomial uses std::unordered_map to keep the pair of a Monomial and its exponent. Can we change the string comparison in assertSame, str(lhs) == str(rhs), to something like lhs.EqualTo(rhs) (only if lhs/rhs have EqualTo?) This should make the testing more robust.

Done. Looks better now, thanks!

---

bindings/pydrake/util/containers.py, line 7 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I don't see why EqualToDict would do this, when regular dict doesn't? If the value type disobeys the __hash__ contract, that's a bug with the value type, not something to work around here.

Done.

---

bindings/pydrake/util/containers.py, line 18 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI Does this deserve a string message? Or is it unreachable?

Done.

---

bindings/pydrake/util/containers.py, line 28 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The meaning of tmp is unclear. (I suggest giving it a better name.)

Done.

---

bindings/pydrake/util/containers.py, line 28 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The meaning of key_wrap and key_unwrap are unclear. I guess they are both supposed to quack like single-argument functors?

Done.

---

bindings/pydrake/util/deprecation.py, line 143 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I think this comment needs to be about 10x larger, explaining what is going on, why we choose to filter it, whether certain numpy versions are affected, etc. -- basically everything you've learned about these deprecation things should be dumped into the comment here. As it stands, I have no way to reason about the justification for this code, nor any idea how to explore or test it.

Done.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 3 of 4 files at r6.
Review status: 7 of 8 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[jwnimmer-tri]

platform

---

Reviewed 1 of 4 files at r6.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

bindings/pydrake/test/symbolic_test.py, line 886 at r5 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Done. Looks better now, thanks!

Should this comment still be here? At least, I don't understand what it means now.

---

bindings/pydrake/test/symbolic_test.py, line 41 at r6 (raw file):

        self.assertTrue(type(rhs) in RHS_TYPES)

    def assertSame(self, lhs, rhs):

FYI Consider renaming this to assertStructurallyEqual to be more precise (that's the term that Expression uses for this comparison).

---

bindings/pydrake/test/symbolic_test.py, line 477 at r6 (raw file):

        with self.assertRaises(RuntimeError) as cm:
            value = bool(e_x == e_x)
        msg = cm.exception.message

BTW abbreviation msg

---

bindings/pydrake/test/symbolic_test.py, line 885 at r6 (raw file):

        p += 3
        # N.B. For whatever reason, the ordering between these two is not the
        # same. Without `ToExpression`, we get an erorr that

BTW typo erorr.

---

Comments from Reviewable

[soonho-tri]

Reviewed 4 of 4 files at r6.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

bindings/pydrake/test/symbolic_test.py, line 37 at r6 (raw file):

class SymbolicTestCase(unittest.TestCase):
    def _check_operands(self, lhs, rhs):

BTW, to be precise, _check_operand_types?

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 7 of 8 files reviewed at latest revision, 5 unresolved discussions.

---

bindings/pydrake/test/symbolic_test.py, line 886 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Should this comment still be here? At least, I don't understand what it means now.

Done. Oops!

---

bindings/pydrake/test/symbolic_test.py, line 37 at r6 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

BTW, to be precise, _check_operand_types?

Done.

---

bindings/pydrake/test/symbolic_test.py, line 41 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI Consider renaming this to assertStructurallyEqual to be more precise (that's the term that Expression uses for this comparison).

Done.

---

bindings/pydrake/test/symbolic_test.py, line 477 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW abbreviation msg

Done.

---

bindings/pydrake/test/symbolic_test.py, line 885 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW typo erorr.

Done. (Removed)

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 1 of 1 files at r7.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

Comments from Reviewable

[soonho-tri]

Reviewed 2 of 2 files at r8.
Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable