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

Introduction of Barriers, Position barrier #87

Merged
merged 108 commits into from
May 29, 2024

Conversation

domrachev03
Copy link
Contributor

This PR is first from the series of PRs, aimed at introduction of Control Barrier Functions, mentioned in #86 . introduces safety sets in the form of Control Barrier Function (CBF) in the presented framework.

For the theory, see our fork, particularly this Jupyter Notebook.

As for implementation, each Barrier defines constraint, and might also define penalization term, if corresponding weight is non-zero. To do this, each barrier has to be able to compute it's value, jacobian, and (optionally) recovery velocity.

Features:

  1. General notion of barrier.
  2. Position barrier for given frame.
  3. Unit tests that cover 100% of presented functionality.

Examples:

  1. Manipulator UR5 with limiting y-plane.
  2. Quadruped Robot Go2 with limiting x and y planes.

Note that this code already fixes problems, mentioned in the #86. Any further suggestions would be welcome

@domrachev03
Copy link
Contributor Author

Good evening. We've fixed most of the suggested points. Moreover, we've started a discussion here.

Waiting for your answer, while @simeon-ned is finalising the documentation on his website.

pink/barriers/barrier.py Outdated Show resolved Hide resolved
pink/barriers/barrier.py Outdated Show resolved Hide resolved
"""All barriers derive from the :class:`Barrier` base class.

The formalism used in this implementation is written down in
examples/barriers/NOTES.md
Copy link
Owner

Choose a reason for hiding this comment

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

Well, I did it there because I did not take the time to adapt it and make the documentation fully self-contained 😊 (That note was written years before Pink v1.)

Feel free to publish the notes somewhere it is more convenient for you 👍 For writing it is more convenient (e.g. an independent note can have its own notations). For new users it is a bit less convenient, as the number of websites to visit and different notations increases. But we can also streamline the documentation later on, when the API is more stable.

pink/solve_ik.py Show resolved Hide resolved
Copy link
Owner

@stephane-caron stephane-caron left a comment

Choose a reason for hiding this comment

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

A quicker review with a question on the 2 factor in a formula.

(The CI test on macOS that doesn't pass is not related to this PR, the issue is tracked in stack-of-tasks/pinocchio#2231 (comment))

@domrachev03
Copy link
Contributor Author

Thank you for your patience and attention, it is great to see so much involvement in the process from you.

We've added a link to extended documentation, and we made an attempt to fix the pipelines (except for mentioned one). If they succeed now, we believe that everything is ready to merge!

Also, since we expect this is not the last MR we would have, we'd like to ask you several quesitons:

  1. As far as we could see, the commits are not squashed at the MR, so is there any style of commit names that we should follow then?
  2. When we are replying to you, should we also create a review, or it's fine to answer via single message?

@stephane-caron
Copy link
Owner

Sounds great 😃

As far as we could see, the commits are not squashed at the MR, so is there any style of commit names that we should follow then?

We don't enforce a particular style. As long as the commit messages are informative and help navigate the change history (in my opinion yours are 👍) that's good to go. When the PR is ready we will merge it into main with a merge commit.

When we are replying to you, should we also create a review, or it's fine to answer via single message?

In PRs there are two kinds of discussions: code-related discussions (associated with code) and free discussions (on the PR page). When reviewers read the code they mostly start code-related discussions. The point of code-related discussions is that we want to make sure all of them are marked as "resolved" before merging the PR.

Thus you don't need to create a review when you reply. It is fine to reply separately in each code-related discussion, as you have done. And if there is something else (maybe not code-related) to discuss you can shoot a single message on the PR page separately.

@domrachev03
Copy link
Contributor Author

We have tried to fix the pipeline yet again, but it is hard to check documentation build, since we have to wait for your approval each time to see a result. The linting pipeline should be fine.

Do you have any suggestion on a better way of repairing documentation pipeline apart from fixing errors one by one?

@stephane-caron
Copy link
Owner

Yes, pipelines have to be approved for first-time contributors. After this PR is merged you won't need approval any more for the following ones.

Don't worry about the documentation, I can fix it quickly afterwards. Let me know when everything else is ready, then I will roundup all code-related discussions and move ahead with the merge.

@domrachev03
Copy link
Contributor Author

If documentation could be fixed later, then everything else is ready!

@stephane-caron
Copy link
Owner

stephane-caron commented May 29, 2024

Alright, let's move forward. Thank you both for this first contribution! Looking forward to the next barriers 😃

@stephane-caron stephane-caron merged commit c9c3f0f into stephane-caron:main May 29, 2024
4 of 12 checks passed
@domrachev03 domrachev03 deleted the pr/position_barrier branch June 24, 2024 21:06
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.

4 participants