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

Removes the ill-fated Trollius support from PlanningMethod. #278

Merged
merged 7 commits into from
Mar 30, 2016

Conversation

psigen
Copy link
Member

@psigen psigen commented Mar 25, 2016

Trollius support never actually worked very well (or at all) for our planners, and with the addition of prpy.futures we have a much better interface for doing parallel operations.

This removes the defer options on all prpy planning methods, and replaced the Ranked implementation with one that uses prpy.futures and plain threading instead.

Trollius support never actually worked very well (or at all), and
with the addition of `prpy.futures` we have a much better interface
for doing parallel operations.

This removes the `defer` options on all prpy planning methods, and
replaced the `Ranked` implementation with one that uses `prpy.futures`
and plain threading instead.
if len(dof_indices) and affine_dofs:
raise ValueError(
'Trajectory contains both affine and regular DOFs.')
# Special case for timing affine-only trajectories.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing newline before this comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand, since I just re-indented this code without changing it.

Where do you want a newline? Do you want a blank line before an elif?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks a bit odd to have no newline between the ValueError and this comment. I suppose it's fine, though, because we do this all the time on elifs.

@mkoval
Copy link
Member

mkoval commented Mar 26, 2016

This is a fantastic change - our ill-fated attempt at using Trollius introduced a lot of needless complexity into the planning pipeline. I am grateful that you removed it and left some specific comments above.

@jeking04 was attempting to use defer=True earlier this week. I'd like to check with her before merging this pull request to see which features, if any, she is currently using.

@jeking04
Copy link
Contributor

I am good with merging this. I couldn't ever quite figure it out and ended
up just using threading around the execute trajectory call rather than the
defer flag.

On Saturday, March 26, 2016, Michael Koval notifications@github.com wrote:

This is a fantastic change - our ill-fated attempt at using Trollius
introduced a lot of needless complexity into the planning pipeline. I am
grateful that you removed it and left some specific comments above.

@jeking04 https://github.com/jeking04 was attempting to use defer=True
earlier this week. I'd like to check with her before merging this pull
request to see which features, if any, she is currently using.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#278 (comment)

Jen King

@mkoval
Copy link
Member

mkoval commented Mar 27, 2016

Thanks for the input, @jeking04! It sounds like you agree that this was an ill-fated endeavor. 😓

I am good with merging this after #282.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants