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

Re-create iris+quadrotor mi-convex planning examples #6243

Open
jwnimmer-tri opened this Issue Jun 2, 2017 · 7 comments

Comments

Projects
None yet
5 participants
@jwnimmer-tri
Collaborator

jwnimmer-tri commented Jun 2, 2017

PR #6235 removed proposed removing Drake's MATLAB implementation the iris+quadrotor mi-convex planning examples. We should re-create these examples.

Possibly also consider anything else in that PR that should be re-created.

@jwnimmer-tri

This comment has been minimized.

Show comment
Hide comment
@jwnimmer-tri

jwnimmer-tri Jun 5, 2017

Collaborator

When PR #6235 proposed removing the MATLAB Quadrotor example, @rdeits reponded:

I'm actually not happy about this. I've cited Drake as the source for this algorithm in the original paper, and now it's being deleted with no replacement. Is this code actually hurting anything? Removing it will make the paper harder to follow without a clear benefit.

(to clarify, I'm referring to the MI-convex quadrotor planning code)

The reason for removing MATLAB Quadrotor code is that Drake's older mechanisms for MATLAB support are being removed. In particular, RigidBodyManipulator.m (and its underlying mex support based on RigidBodyTree) will be removed -- and to my understanding, the Quadrotor relies on that module.

I can hold off on the Quadrotor removal as long as possible if that helps, but that may only buy us 1-2 weeks, depending on how fast other turn-down PRs are moving.

In terms of keeping paper citations useful, I am open to suggestions. The easy starting point would be to add a drake/examples/Quadrotor/README.md that explained how to find the prior software (e.g., it could hyperlink to https://github.com/RobotLocomotion/drake/tree/last_sha_with_original_matlab/drake/examples/Quadrotor). Alternatively, we could have a drake/examples/Quadrotor/doc/ or drake/examples/Quadrotor/convex_planning_reference/ folder that kept a copy of some matlab files for inspection, but they would no longer run.

Collaborator

jwnimmer-tri commented Jun 5, 2017

When PR #6235 proposed removing the MATLAB Quadrotor example, @rdeits reponded:

I'm actually not happy about this. I've cited Drake as the source for this algorithm in the original paper, and now it's being deleted with no replacement. Is this code actually hurting anything? Removing it will make the paper harder to follow without a clear benefit.

(to clarify, I'm referring to the MI-convex quadrotor planning code)

The reason for removing MATLAB Quadrotor code is that Drake's older mechanisms for MATLAB support are being removed. In particular, RigidBodyManipulator.m (and its underlying mex support based on RigidBodyTree) will be removed -- and to my understanding, the Quadrotor relies on that module.

I can hold off on the Quadrotor removal as long as possible if that helps, but that may only buy us 1-2 weeks, depending on how fast other turn-down PRs are moving.

In terms of keeping paper citations useful, I am open to suggestions. The easy starting point would be to add a drake/examples/Quadrotor/README.md that explained how to find the prior software (e.g., it could hyperlink to https://github.com/RobotLocomotion/drake/tree/last_sha_with_original_matlab/drake/examples/Quadrotor). Alternatively, we could have a drake/examples/Quadrotor/doc/ or drake/examples/Quadrotor/convex_planning_reference/ folder that kept a copy of some matlab files for inspection, but they would no longer run.

@jamiesnape

This comment has been minimized.

Show comment
Hide comment
@jamiesnape

jamiesnape Jun 6, 2017

Contributor

FWIW IRIS and its Python bindings build with Bazel easily enough (rdeits/iris-distro#57) if Python were a suitable alternative to MATLAB.

Contributor

jamiesnape commented Jun 6, 2017

FWIW IRIS and its Python bindings build with Bazel easily enough (rdeits/iris-distro#57) if Python were a suitable alternative to MATLAB.

@edrumwri

This comment has been minimized.

Show comment
Hide comment
@edrumwri

edrumwri Jun 6, 2017

Contributor
Contributor

edrumwri commented Jun 6, 2017

@rdeits

This comment has been minimized.

Show comment
Hide comment
@rdeits

rdeits Jun 6, 2017

Contributor

1 or 2 weeks doesn't matter either way, so if RigidBodyManipulator is going then I guess the Quadrotor code is going too. Having a readme with a link to the relevant branch would be nice.

@jamiesnape I would love to have the example in Python rather than Matlab. But converting it would be equivalent to rewriting it from scratch, since the APIs are all different

@edrumwri yeah, lesson learned.

But I have to say that I'm still not happy with this situation. My understanding was that the high-level APIs in Matlab would remain, but be powered by the new C++ core. Deleting RigidBodyManipulator without any replacement means that a great deal of Drake's scientific content will cease to work, and it risks alienating Drake's long-time users.

Contributor

rdeits commented Jun 6, 2017

1 or 2 weeks doesn't matter either way, so if RigidBodyManipulator is going then I guess the Quadrotor code is going too. Having a readme with a link to the relevant branch would be nice.

@jamiesnape I would love to have the example in Python rather than Matlab. But converting it would be equivalent to rewriting it from scratch, since the APIs are all different

@edrumwri yeah, lesson learned.

But I have to say that I'm still not happy with this situation. My understanding was that the high-level APIs in Matlab would remain, but be powered by the new C++ core. Deleting RigidBodyManipulator without any replacement means that a great deal of Drake's scientific content will cease to work, and it risks alienating Drake's long-time users.

@RussTedrake

This comment has been minimized.

Show comment
Hide comment
@RussTedrake

RussTedrake Jun 7, 2017

Contributor

@rdeits Someone whose opinion I value very much convinced me about two years ago that the code we'd been developing was formatted/packaged in a way that made it inaccessible to many and essentially unusable by anyone working with real robots. I agree it serves as a runnable example for those reading and understanding the papers, and that remains true (on the old sha).

I'm personally committed to taking the scientific content developed here to maturity in the new framework because I believe that it can and should be used by many. (I asked Jeremy to create this issue and assign it to me). I think that the new code is far enough along now that we're ready to zap and port.. and not zapping causes at least two major issues: 1) users find and experiment with the old api which is now considered deprecated and unsupported, and 2) the existence of the old API is blocking the completion of the transition to bazel and causes other woes (like build times which exceed the time limit on the free instances of travis).

Contributor

RussTedrake commented Jun 7, 2017

@rdeits Someone whose opinion I value very much convinced me about two years ago that the code we'd been developing was formatted/packaged in a way that made it inaccessible to many and essentially unusable by anyone working with real robots. I agree it serves as a runnable example for those reading and understanding the papers, and that remains true (on the old sha).

I'm personally committed to taking the scientific content developed here to maturity in the new framework because I believe that it can and should be used by many. (I asked Jeremy to create this issue and assign it to me). I think that the new code is far enough along now that we're ready to zap and port.. and not zapping causes at least two major issues: 1) users find and experiment with the old api which is now considered deprecated and unsupported, and 2) the existence of the old API is blocking the completion of the transition to bazel and causes other woes (like build times which exceed the time limit on the free instances of travis).

@rdeits

This comment has been minimized.

Show comment
Hide comment
@rdeits

rdeits Jun 7, 2017

Contributor

I still agree with two-years-ago-me, so if you think this is the right time, then I'm OK with it. I do like @jwnimmer-tri 's suggestion of adding a readme in the relevant deleted example folders, just so that things a new user might do (like a github repo search for "quadrotor" or just browsing to the examples folder) will still point them to the right SHA.

Contributor

rdeits commented Jun 7, 2017

I still agree with two-years-ago-me, so if you think this is the right time, then I'm OK with it. I do like @jwnimmer-tri 's suggestion of adding a readme in the relevant deleted example folders, just so that things a new user might do (like a github repo search for "quadrotor" or just browsing to the examples folder) will still point them to the right SHA.

@jwnimmer-tri

This comment has been minimized.

Show comment
Hide comment
@jwnimmer-tri

jwnimmer-tri Jun 7, 2017

Collaborator

Yup, I'm definitely going to add a Quadrotor README about where to find this particular feature in the history.

The idea of adding breadcrumbs for other particular examples is interesting; I'll use #6268 to discuss it.

Collaborator

jwnimmer-tri commented Jun 7, 2017

Yup, I'm definitely going to add a Quadrotor README about where to find this particular feature in the history.

The idea of adding breadcrumbs for other particular examples is interesting; I'll use #6268 to discuss it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment