[systems] LuenbergerObserver and KalmanFilter use shared_ptr #22351
Conversation
jwnimmer-tri
added
priority: low
release notes: feature
|
+@RussTedrake for feature review please, if you have availability. If not that's fine and I'll find a different reviewer after the new year. |
There was a problem hiding this comment.
Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: needs at least two assigned reviewers
systems/estimators/luenberger_observer.cc line 87 at r1 (raw file):
: LuenbergerObserver(std::shared_ptr<const System<T>>( /* managed object = */ std::shared_ptr<void>{}, /* stored pointer = */ &observed_system),
btw -- TIL. Nice. That certainly simplifies things! We have a lot of places where we have parallel owned vs referenced workflows.
There was a problem hiding this comment.
Q: How is it that the tests which pass a unique_ptr to the constructor still pass? Which codepath are they taking?
Reviewable status: needs at least two assigned reviewers
|
(and should we include a test which explicitly calls the shared_ptr versions?) |
The LuenbergerObserver and KalmanFilter now take the observed system as a shared_ptr-to-const, not unique_ptr-to-mutable. The functions don't need to mutate anything and don't need exclusive ownership, and passing by unique_ptr causes trouble in our pydrake bindings. Overload SteadyStateKalmanFilter to accept the context by-const-ref instead of the unnecessary by-mutable-unique_ptr.
jwnimmer-tri
force-pushed
the
kalman-shared
branch
from
bd7e1a7 to
7ef27d3
Compare
There was a problem hiding this comment.
Q: How is it that the tests which pass a unique_ptr to the constructor still pass? Which codepath are they taking?
The shared-ptr class has a conversion constructor that accepts a unique_ptr. See overload (13) at https://en.cppreference.com/w/cpp/memory/shared_ptr/shared_ptr.
(and should we include a test which explicitly calls the shared_ptr versions?)
I ported the tests to shared_ptr and added coverage for the new const-context overload for the kalman filter.
Reviewable status: needs at least two assigned reviewers
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: needs at least two assigned reviewers
|
+@SeanCurtis-TRI for platform review per schedule, please. |
There was a problem hiding this comment.
Reviewed 5 of 5 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status:complete! all discussions resolved, LGTM from assignees SeanCurtis-TRI(platform),RussTedrake(platform)
Towards #5842 and #21968. We must stop passing
unique_ptras an argument from Python down to C++... passingunique_ptrin an argument of a call from Python into C++ is made possible by Drake-specific hacks to pybind11, and we need to get rid of the hacks.This change is