drake
drake copied to clipboard
RobotLocomotion
Bind GcsTrajectoryOptimization::EstimateComplexity
Very tiny PR -- I wanted to use this function for something in python, and figured there's a chance others might want to as well.
This change is 
No preference. It's such a tiny change, pretty much anyone could do it? Is this an instance of a "single reviewer okay" PR?
wait... why do you want to use this function? I actually don't like that function; and thought it was only there for testing purposes?
As I'm running multiple GCS programs of different sizes and levels of connectivity, I wanted a better way to compare their complexity. It's not super important, but since it was publicly available in the C++, I figured I could add a quick Python binding.
With my platform reviewer hat on:
At the moment, this PR is internally inconsistent because it binds a function for end-users whose docs say it's not intended for end-users. (The API comment is somewhat ambiguous, though.)
I think we're on a saddle with two ways out:
(1) Clarify the ambiguous API comment by removing the "for regression testing purposes" text. Add the Python binding. Given https://github.com/RobotLocomotion/drake/pull/19444#pullrequestreview-1464885672, it seems like you might also need to convince Russ about a better function name, and/or an improved implementation.
(2) Clarify the ambiguous API comment by strengthening "for regression testing purposes" to more directly say "Internal use only", and hiding the function from Doxygen. If y'all decide to go this route, LMK and I can prepare a new PR with the right Doxygen giblets for this approach.
\CC @ggould-tri FYI as reviewer on the prior PR.
I agree with Russ that the function should be improved. Full disclosure, in my fork I use a binding of this function to set the number of super basics SNOPT should use. There is use in an improved version that is not only for internal use.
The function was initially introduced because David was PR'ing an implementation-only change (no API change, no functionality change), citing that the problem formulation got smaller, and we wanted a strategy to confirm that in a test.
The conversation in this thread suggests to me that it is already being interpreted as having a meaningful correspondence with GCS solve times. I'm not at all convinced that it does. (I could be convinced, and could definitely believe that it would be valuable if that correlation was available). My first preference would be to become convinced. My second preference would be to mark it explicitly internal (or even remove it entirely).
Having some sort of measure of the complexity of a mathematical program is super useful. Even if it's only weakly monotonic, being able to compare two programs and say "this one is much harder than the other" is very nice. It doesn't have to be this method, and if there's a better method, I'm happy to use it. But for now, this is what's in GCS.
Closing per author's note above; reopen or duplicate the next time someone wants this feature.
