pydrake: math overloads provide inconsistent user experience

[RussTedrake]

Consider the following example:

from pydrake.all import AutoDiffXd, Variable

x = 1
y = AutoDiffXd(1, [1, 0])
z = Variable('z')

Here's a simple basic use case, which does work fine.

import pydrake.math

print(pydrake.math.sin(x))
print(pydrake.math.sin(y))
print(pydrake.math.sin(z))

returns

0.8414709848078965
AD{0.8414709848078965, nderiv=2}
sin(z)

But there are some nearby patterns that lay a trap for users:

import numpy as np

print(np.sin(x))
print(np.sin(y))
print(np.sin(z))

gives

0.8414709848078965
AD{0.8414709848078965, nderiv=2}
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
AttributeError: 'pydrake.symbolic.Variable' object has no attribute 'sin'

The above exception was the direct cause of the following exception:

TypeError                                 Traceback (most recent call last)
<ipython-input-206-4a0ca175cc23> in <module>
      1 print(np.sin(x))
      2 print(np.sin(y))
----> 3 print(np.sin(z))

TypeError: loop of ufunc does not support argument 0 of type pydrake.symbolic.Variable which has no callable sin method

Q: Can we teach Expression to understand the numpy version?

Here is another one:

from pydrake.all import sin

print(sin(x))
print(sin(y))
print(sin(z))

gives

0.8414709848078965
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-207-74707ae6920c> in <module>
      1 print(sin(x))
----> 2 print(sin(y))
      3 print(sin(z))

TypeError: sin(): incompatible function arguments. The following argument types are supported:
    1. (arg0: pydrake.symbolic.Expression) -> pydrake.symbolic.Expression

Invoked with: <AutoDiffXd 1.0 nderiv=2>

Q: Can we move the math import in all.py below the symbolic import, so the symbolic version doesn't clobber the more general one?
(I know that we're in the land of perhaps ill-advised pydrake.allness, but I believe that should be better for non-expert users).

cc @EricCousineau-TRI . @soonho-tri -- might you be willing to take a look?

[soonho-tri]

Q: Can we teach Expression to understand the numpy version?

it's a matter of adding sin method (and other corresponding methods such as cos) to the Expression class (in Python). When it's there, numpy will call Expression.sin(). I'll open a PR.

[soonho-tri]

@EricCousineau-TRI , what is the best way to add a new method foo to a class created A via PyBind? Is it OK to use setattr?

def foo(self):
    print('hello world!')
setattr(A, 'foo', foo)

Update: never mind. I think it's better to add it from C++.

[EricCousineau-TRI]

FWIW No need to use setattr; you can simply do A.foo = foo ;)

[soonho-tri]

@EricCousineau-TRI , we still have a remaining issue here. The following code gives TypeError:

from pydrake.all import AutoDiffXd, Variable, sin

x = 1
y = AutoDiffXd(1, [1, 0])
z = Variable('z')

print(sin(x))
print(sin(y))
print(sin(z))

---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-207-74707ae6920c> in <module>
       print(sin(x))
---->  print(sin(y))
       print(sin(z))

TypeError: sin(): incompatible function arguments. The following argument types are supported:
    1. (arg0: pydrake.symbolic.Expression) -> pydrake.symbolic.Expression

Invoked with: <AutoDiffXd 1.0 nderiv=2>

As @RussTedrake wrote above, one solution is to update all.py:

Q: Can we move the math import in all.py below the symbolic import, so the symbolic version doesn't clobber the more general one?
(I know that we're in the land of perhaps ill-advised pydrake.allness, but I believe that should be better for non-expert users).

To be specific, here is a patch:

modified   bindings/pydrake/all.py
@@ -30,10 +30,10 @@ from . import getDrakePath
 from .autodiffutils import *
 from .forwarddiff import *
 from .lcm import *
-from .math import *
 from .perception import *
 from .polynomial import *
 from .symbolic import *
+from .math import *
 from .trajectories import *

What do you think about this approach?

[jwnimmer-tri]

I am not a huge fan of playing games with import order. If we have multiple competing definitions of a name and we need a specific one of those definitions to win, then we should explicitly disavow the unwanted implementations, i.e., we should not even attempt to import the unwanted implementations.

In fact, it should be a test failure if any of the "all.py" files ever overwrites a previously-imported name. When there are multiple definitions in play, we need to ask the drake developer to choose which one is supposed to win, instead of leaving it to alphabetical chance.

[RussTedrake]

I'm not sure how hard that is to achieve. But the pydrake.math variants are the ones that should be imported in all.py. I'm all for doing things the right way, but perhaps we can land the small fix immediately and open an issue to track the double import defect?

[EricCousineau-TRI]

I'm onboard with polishing later, but #15052 looks like a great intermediate solution!