-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Implement IRIS-NP2 #23001
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
base: master
Are you sure you want to change the base?
Implement IRIS-NP2 #23001
Conversation
…lision filters to the collision pairs setup.
+@RussTedrake for feature review |
@drake-jenkins-bot mac-arm-sonoma-clang-bazel-experimental-release, please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small questions/requests below.
The code is beautiful. It's hard to check all of the details/logic, but I did try to do a pretty careful read through.
thanks for doing this!
Reviewed 11 of 11 files at r1, all commit messages.
Reviewable status: 7 unresolved discussions, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @cohnt)
planning/iris/iris_np2.h
line 60 at r1 (raw file):
volume-in-collision is larger than ε Pr[λ(P\Cfree)/λ(P) > ε] ⋞ δ.
I assume that your use of ⋞ is intentional, but it's atypical. Does \le really not work?
planning/iris/iris_np2.h
line 88 at r1 (raw file):
solving this function. See AcquireLicense for more details. This feature is still in development, so certain features are not yet supported.
nit: you're using features twice in two different ways in the same sentence.
planning/iris/iris_common.h
line 236 at r1 (raw file):
namespace internal { // See Definition 1 in the paper.
this is in iris_common.h. I don't think it's clear which paper you're referring to here.
planning/iris/test/iris_np2_test.cc
line 22 at r1 (raw file):
using symbolic::Variable; // Reproduced from the IrisInConfigurationSpace unit tests.
is there a plan (perhaps a TODO) to share more of the code between the test files?
planning/iris/iris_np2.cc
line 51 at r1 (raw file):
for (const auto& pair : sorted_pairs) { auto query_object = plant.get_geometry_query_input_port() .template Eval<QueryObject<double>>(context);
Hopefully the caching makes this actually ok, but I think it's more clear/clean if it's outside of the for loop?
planning/iris/iris_np2.cc
line 66 at r1 (raw file):
} /* Check if any unsuppoorted features have been used, as well as any other
nit: typo: unsupported
planning/iris/iris_np2.cc
line 205 at r1 (raw file):
DRAKE_DEMAND(body_B_ptr != nullptr); if (!checker.IsCollisionFilteredBetween(*body_A_ptr, *body_B_ptr)) {
i don't understand something here. i thought GetCollisionCandidates should not return any pairs that are already filtered?
so I don't understand the point of this loop (and the difference between pairs and possible_pairs?
I assume there is something I've missed, but then perhaps you need to explain it to me (and future readers) by documenting it around here?
This adds the minimal functionality for IRIS-NP2 to begin being used. Additional features will be added following this PR, but we're already over 1000 lines. This does not close #21822.
This change is