RobotLocomotion / drake

Model-based design and verification for robotics.
https://drake.mit.edu
Other
3.27k stars 1.26k forks source link

from pydrake.all import Polynomial now imports polynomial.Polynomial instead of symbolic.Polynomial #18353

Closed RussTedrake closed 1 year ago

RussTedrake commented 1 year ago

What happened?

An old notebook failed with an unexpected error, and I've realized that this was the problem. I have not yet bisected to find the offending commit, but in addition to being a breaking change, I think symbolic is the Polynomial that we want.

We used to have

from pydrake.all import Polynomial

was equivalent to

from pydrake.symbolic import Polynomial

but now it gets

from pydrake.polynomial import Polynomial

Version

No response

What operating system are you using?

No response

What installation option are you using?

No response

Relevant log output

No response

EricCousineau-TRI commented 1 year ago

but in addition to being a breaking change

This is established in pydrake.alls warning about its fragile / unstable contract: https://drake.mit.edu/pydrake/pydrake.all.html

Import order matters! If there is a name conflict, the last one imported wins.

The best thing to do is not use pydrake.all if you want stability. As workaround, we can shuffle ordering, but I don't really want to spend too much brainpower fixing other stuff like this in the future.

Does this make sense?

EricCousineau-TRI commented 1 year ago

Ah, also, I added a utility to anzu, ~pydrake_all_rewrite~ pydrake_import_rewrite. I'd recommend using that if you want to quickly transcribe the imports. Can you try that out and let me know?

We can hoist it, but I'd certainly request that someone else drive the hoisting. I'm happy to review.

RussTedrake commented 1 year ago

I'm very aware that pydrake.all isn't the official recommendation, but i've settled into using it until we address #11802. It's not because I'm lazy, but because the full spelling is intimidating for non-experts and hard for them to work with.

The point is that the ordering must have changed, and I consider it a regression.

RussTedrake commented 1 year ago

My first check made it look like it was probably the combination of #16067 with #15052... but I have run this notebook much more recently than that... Hrmm..

RussTedrake commented 1 year ago

I did a proper bisect and the change happened in https://github.com/RobotLocomotion/drake/pull/18160 by @jwnimmer-tri .

EricCousineau-TRI commented 1 year ago

It's not because I'm lazy, but because the full spelling is intimidating for non-experts and hard for them to work with. The point is that the ordering must have changed, and I consider it a regression.

Acknowledged, but this still implies a desired level of stability. At present, our commitment is "unstable", so it's hard to judge it as a regression by means of documentation, esp. with a lack of testing.

But I understand your point about wanting to keep pydrake.all convenient. For the sake of forward progress, I'm A-OK adding some testing and instrumentation for "Preferred Ordering". Will submit PR with intent to establish expectations that entail minimum work (that is, zero deprecation upon a new preferred ordering).

RussTedrake commented 1 year ago

I agree. That's perfect. Thank you!

In this case, the real fix is to resolve #7576, but all I would hope for now is a test confirming your favorite way to write

from pydrake.all import Polynomial as all_poly
from pydrake.symbolic import Polynomial as sym_poly
assert(all_poly == sym_poly)

and a hopefully minimal change so that the test passes.

I was planning to do it if you don't want to, but if you're willing, then I suspect you might implement the change better than me.

EricCousineau-TRI commented 1 year ago

Went ahead and submitted #18357, but thank you for offering!