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

pydrake systems: Add missing bindings to ContinuousState, DiscreteValues, VectorBase, and BasicVector #14231

Merged
merged 1 commit into from
Oct 21, 2020

Conversation

mpetersen94
Copy link
Contributor

@mpetersen94 mpetersen94 commented Oct 21, 2020

Addresses two check boxes for #14223


This change is Reviewable

@mpetersen94
Copy link
Contributor Author

@RussTedrake for feature review
I was bumping up against these too so decided to cross them off the list.

Copy link
Contributor

@RussTedrake RussTedrake left a comment

Choose a reason for hiding this comment

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

awesome. thanks! and looks perfect to my eyes. :lgtm:
+@sammy-tri for platform review.

Reviewed 4 of 4 files at r1.
Reviewable status: LGTM missing from assignee sammy-tri(platform) (waiting on @sammy-tri)

@mpetersen94
Copy link
Contributor Author

I just realized that the way I wrote the operator[] binding allows you to use it for accessing values but not for setting values. How much does that impede your use case Russ?

Copy link
Contributor

@sammy-tri sammy-tri left a comment

Choose a reason for hiding this comment

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

:lgtm: Would you prefer that I wait for Russ's feedback on the operator[] question before merging?

Reviewed 4 of 4 files at r1.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees sammy-tri(platform),RussTedrake(platform)

@mpetersen94
Copy link
Contributor Author

Yes, let's wait on merge because I just found a fix to the problem. Adding tests and will post shortly.

Copy link
Contributor

@sammy-tri sammy-tri left a comment

Choose a reason for hiding this comment

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

+(status: do not merge) adding Do Not Merge just to make sure I don't do it accidentally while you're updating.

Reviewable status: labeled "do not merge"

Copy link
Contributor Author

@mpetersen94 mpetersen94 left a comment

Choose a reason for hiding this comment

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

Ok. Ready for review and merge if everyone is happy. I also added bidings for DiscreteValues while it was compiling.

Reviewable status: labeled "do not merge" (waiting on @RussTedrake and @sammy-tri)

@mpetersen94 mpetersen94 changed the title pydrake systems: Add missing bindings to ContinuousState, VectorBase, and BasicVector pydrake systems: Add missing bindings to ContinuousState, DiscreteValues, VectorBase, and BasicVector Oct 21, 2020
Copy link
Contributor

@sammy-tri sammy-tri left a comment

Choose a reason for hiding this comment

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

-(status: do not merge) Just one question about a doc comment.

Reviewed 5 of 5 files at r2.
Reviewable status: 1 unresolved discussion, labeled "do not merge" (waiting on @mpetersen94)


systems/framework/discrete_values.h, line 60 at r2 (raw file):

  /// Constructs a %DiscreteValues that owns the underlying @p data. Every entry
  /// must be non-null.
  /// @exclude_from_pydrake_mkdoc{This overload is not bound in pydrake.}

Is this also true for the overload directly above? (1arg data)

Copy link
Contributor

@RussTedrake RussTedrake left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 5 of 5 files at r2.
Reviewable status: 1 unresolved discussion (waiting on @mpetersen94)

Copy link
Contributor Author

@mpetersen94 mpetersen94 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion (waiting on @RussTedrake and @sammy-tri)


systems/framework/discrete_values.h, line 60 at r2 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

Is this also true for the overload directly above? (1arg data)

Done. Initially I tried binding the other which caused some seg faults. Binding this one and excluding the other works better.

Copy link
Contributor

@sammy-tri sammy-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r3.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees sammy-tri(platform),RussTedrake(platform)

@sammy-tri sammy-tri merged commit 5f8d923 into RobotLocomotion:master Oct 21, 2020
@mpetersen94 mpetersen94 deleted the bind/systems branch October 28, 2020 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants