Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[py mp] MathematicalProgram.AddLinearConstraint prefers sparse overload? #20516

Closed
EricCousineau-TRI opened this issue Nov 10, 2023 · 13 comments · Fixed by #20524
Closed

[py mp] MathematicalProgram.AddLinearConstraint prefers sparse overload? #20516

EricCousineau-TRI opened this issue Nov 10, 2023 · 13 comments · Fixed by #20524
Assignees
Labels
component: pydrake Python API and its supporting Starlark macros type: bug

Comments

@EricCousineau-TRI
Copy link
Contributor

What happened?

I have a torque controller written as a QP in Python.
The code creates dense NumPy matrices for use with prog.AddLinearConstraint().

When profiling, I find that it tends to use the sparse flavor.
I found this by using py-spy, and seeing calls into scipy.sparse taking up a nontrivial amount of time.

Not sure why this happens; best guess is that the pybind11 binding overload for sparse matrices has better "affinity" (i.e., can accept arguments with no conversion), though not sure why this would be the case.

For now, I can workaround it by patching Drake to not bind those overloads.

\cc @hongkai-dai @jwnimmer-tri

Version

5303029

What operating system are you using?

No response

What installation option are you using?

No response

Relevant log output

No response

@EricCousineau-TRI EricCousineau-TRI added type: bug component: pydrake Python API and its supporting Starlark macros labels Nov 10, 2023
@EricCousineau-TRI EricCousineau-TRI self-assigned this Nov 10, 2023
@jwnimmer-tri
Copy link
Collaborator

jwnimmer-tri commented Nov 10, 2023

This might a regression due to #20361. \CC @AlexandreAmice

That PR had unit tests that directly construct a LinearConstraint that (seem to) prove that the dense vs sparse overloading happens correctly.

However, the unit tests for MathematicalProgram only (seem to) prove that sparse overloads are correctly selected -- there were no checks added that is_dense_A_constructed() was True when adding dense A matrices.

@jwnimmer-tri
Copy link
Collaborator

@EricCousineau-TRI maybe you could share exactly which type you were passing at the call site (and the types of any other args that might affect the overload selection)?

@EricCousineau-TRI
Copy link
Contributor Author

Yeah, they were all np.array(dtype=float64) for numerics, and np.array(dtype=object) (Variable). Here's a snapshot of one of the solves: #20521 (1c55fec)

@EricCousineau-TRI

This comment was marked as outdated.

@EricCousineau-TRI

This comment was marked as outdated.

@EricCousineau-TRI
Copy link
Contributor Author

EricCousineau-TRI commented Nov 10, 2023

From #20519 (comment), seems like one issue (for testing) is that scipy_stub in Drake has different behavior than actual scipy, at least the version that I have on my system.

Still not clear why overloads prefer sparse version, though.

@jwnimmer-tri
Copy link
Collaborator

Sounds like we have a repro in hand now. I'll offer the next step might be to bisect Drake to figure out which commit caused this.

@EricCousineau-TRI
Copy link
Contributor Author

I actually now have a pure pybind11 repro, so the bisection would be against our fork itself:
77c2e56

It could either be the upgrade to pybind11, or it could always have been a problem.

Will check on the pybind11 side.

@EricCousineau-TRI
Copy link
Contributor Author

(The issue occurs when an overload seems to involve both dense vs. sparse and an additional argument that is dtype=object)

@EricCousineau-TRI

This comment was marked as resolved.

@EricCousineau-TRI

This comment was marked as resolved.

@EricCousineau-TRI
Copy link
Contributor Author

Yeah, I'd posit that this has always been a problem, we just didn't have sufficient test coverage (explicitly checking overload behavior + using actual scipy).
But it ultimately seems like a bug with pybind11 (our fork) itself.

@EricCousineau-TRI
Copy link
Contributor Author

EricCousineau-TRI commented Nov 10, 2023

Attempted to migrate Drake+Bazel repro to pybind11+CMake, but am currently failing to reproduce the same results locally; odd.
RobotLocomotion/pybind11#68
RobotLocomotion/pybind11@1ed181b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: pydrake Python API and its supporting Starlark macros type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants