Breaking Change: Rewrite Controller API to have less templated components#30
Open
Breaking Change: Rewrite Controller API to have less templated components#30
Conversation
I think I have removed it properly from all of the library code. However, nothing will compile at the moment because the examples and unit tests have not been updated. Other changes: - removed commented out code in various places - Add new sampling distribution method to get the optimal control sequence that is located on the GPU. This replaces control_d_ in the Controller. - Adjust Controller::allocateCUDAMemoryHelper() to be less confusing. The input is no longer expected to be the number of systems - 1 and the unused boolean has been removed. Also added freeing of memory before allocating more as we can now adjust the size of some device memory. - Adjust Feedback Controller API a bit to account for more complicated memory management associated with variable number of timesteps.
Fixed various compilation errors and bugs in removing num_timesteps as template parameter
All examples now compile and run correctly. Unit tests are going to be fixed next.
autoformatter script can now take in multiple arguments of directories to run formatting over
Fixed controller_generic_tests first. Still need to update other controller and feedback controller unit tests at minimum and probably others.
It looks like some unit tests are failing due to the changes so those still need fixing.
Other Changes: - Rename internal methods for timesteps and rollouts to setNumTimestepsHelper() to follow existing convention - Update Tube-MPPI and R-MPPI to overwrite setNumTimestepsHelper() and setNumRolloutsHelper() to also resize their nominal trajectory variables - Change default number of rollouts to cause issues if not set correctly - Update library compilations to use new controler template syntax - update examples to use new controller template syntax
Other changes: - fix setNumTimesteps() to use logic to fill in control trajectory as slideControlSequence() - reset control_ to initial control trajectory when new number of timesteps is larger than current timesteps - fix Tube MPPI constructor unit tests - Add unit tests to check control trajectories are both proper size and filled with linear interpolation between last provided control and model's zero control after construction - Add unit tests to check R-MPPI and Tube-MPPI's nominal control trajectories - Add unit tests for setNumRollouts()
…llouts_from_template
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Warning
This pull request removes the MAX_TIMESTEPS and NUM_ROLLOUTS integers from the base Controller template.
Controllers will now only be templated upon the Dynamics, Cost Function, Feedback Controller, Sampling Distribution and Controller Params.
Reasoning
These were originally in place so that we could define various fixed-sized Eigen Matrices like control and state trajectories. However, we never do Eigen operations on the full trajectory in this library (we instead pull out a single state or control vector at a given time and then do computation on those vectors alone) that could leverage the fixed-size and this previous formulation ended up limiting the number of samples or timesteps that could be used with MPPI-Generic due to limitations on how large a fixed-size Eigen Matrix can be. We had a half-measure currently in place where we utilized a
MAX_TIMESTEPStemplate parameter and then allowed the useer to adjust the number of timesteps at run-time as long as it was to a number less than or equal toMAX_TIMESTEPS. This was a half-measure because the underlying data structures were stillMAX_TIMESTEPSlarge and we would just zero out the rest of the trajecotory if the timesteps were set to be lower. With this pull request, we instead switch to definingcontrol_trajectory,state_trajectory, etc. to be dynamically-sized Eigen matrices with the first dimension fixed to the relevant size (CONTROL_DIM,STATE_DIM, etc.) and the second dimension being Dynamic.Instead of template parameters, the base
ControllerParamsstructure fully controls the number of timesteps and rollouts undernum_timesteps_andnum_rollouts_and can be adjusted at run-time. I have added various checks to the controller constructors andsetParams()methods to appropriately resize the relevant variables on both the CPU and GPU when these parameters are adjusted. New controller methodssetNumTimesteps()andsetNumRollouts()have also been added The MPPI, Tube-MPPI, and R-MPPI controllers were all rewritten to follow this new Controller template API. Removing fixed-size Eigen Matrices required a bit of rewriting of the DDP feedback controller which was also depending on compile-time knowledge of the number of timesteps. Finally, examples and unit tests were also rewritten and verified by hand to work with the new Controller API. New unit tests to test adjusting the number of rollouts and timesteps at run-time were also written though I am probably missing some edge cases.Overall, this change should not reduce run-time performance and should allow users more flexibility such as updating the number of samples from a configuration file instead of requiring compilation with every change. Compilation times might also be reduced slightly as there are less template specializations but no promises on that end.