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

Implement Continuous algebraic Riccati equation function #157

Merged
merged 8 commits into from
Dec 7, 2020

Conversation

GiulioRomualdi
Copy link
Member

This PR implements an algorithm to solve the Continuous algebraic Riccati equation.

The main idea has been taken from drake.

cc @Giulero

@GiulioRomualdi GiulioRomualdi self-assigned this Nov 27, 2020
@GiulioRomualdi GiulioRomualdi added the enhancement New feature or request label Nov 27, 2020
Comment on lines 67 to 73
constexpr double symmetricTolerance = 1e-8;
if (!Q.isApprox(Q.transpose(), symmetricTolerance))
{
std::cerr << errorPrefix << "The matrix Q must be symmetric." << std::endl;
result.first = false;
return result;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could possibly be an utility function which could be used in other instances for debugging (for example also in estimators library). What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

symmetricTolerance parameter could be a generic defnition across the framework.

Copy link
Member

@S-Dafarra S-Dafarra Nov 27, 2020

Choose a reason for hiding this comment

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

Also, it might be time-consuming. Consider having a way to skip that, like putting it into an assert.

@GiulioRomualdi
Copy link
Member Author

hI, @S-Dafarra I was trying to transform the function into a class but I've several doubts. I was thinking to organize it as

bool initialize(std::weak_ptr<ParametersHandler::IParametersHandler> handler);

bool setMatrices(Eigen::Ref<const Eigen::MatrixXd> A,
                     Eigen::Ref<const Eigen::MatrixXd> B,
                     Eigen::Ref<const Eigen::MatrixXd> Q,
                     Eigen::Ref<const Eigen::MatrixXd> R);

Solution getSolution();

bool getSolution(Eigen::Ref<Eigen::MatrixXd> S);

however, I don't know how to split the code. For instance, in setMatrices I can just copy the matrices A, B Q and R and I can also create the hamiltonian matrix H.
On the other hand in getSolution() I can run the algorithm to compute S. However, given a set of A B Q and R only one solution exists so it's not efficient to compute the solution in getSolution()

For instance this code will run the algorithm 4 times.

CARE care();
care.setMatrices(A,B,Q,R);

CARE::Solution sol = care.getSolution();
sol = care.getSolution();
sol = care.getSolution();
sol = care.getSolution();
sol = care.getSolution();

So the question is: shall we compute the solution while setting A, B Q and R and store it as a parameter of the class? In this case, shall we add the following matrices as a parameter of the class (notice that the size of H is known only after setMatrices is called)?

Eigen::MatrixXd Z = H;
Eigen::MatrixXd Z_old;

Indeed every time we set new matrices H has to be reset

@S-Dafarra
Copy link
Member

I think you can add a solve method that does the computation. This may also just output a boolean in case of success/failure. Then, the entire pipeline init -> set -> solve -> get could be handled by a single static function, in case you don't care about memory allocation.

@GiulioRomualdi
Copy link
Member Author

I like it

@GiulioRomualdi
Copy link
Member Author

Hi @S-Dafarra and @prashanthr05 I completely restructure the code. Let me know what you think

src/Math/src/CARE.cpp Outdated Show resolved Hide resolved
src/Math/src/CARE.cpp Outdated Show resolved Hide resolved
src/Math/src/CARE.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@prashanthr05 prashanthr05 left a comment

Choose a reason for hiding this comment

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

I also clarified the previous doubt of the Hamiltonian matrix's subblock sign differences from Wikipedia. Turns out the choice used here is also a Hamiltonian matrix because it respects the identity $J Z = (J Z)^T$ where $J = \begin{bmatrix} 0_n & I_n \\ - I_n & 0_n \end{bmatrix}$.
(the requirement of off-diagonal elements being symmetric and the sum of diagonal elements equal to zero is the property).

For me the changes look neat. However, I will also incorporate @S-Dafarra suggestions.

/**
* Initialize the continuous algebraic riccati equation solver.
* @param handler pointer to the parameter handler.
* @note The following parameters are required by the class:
Copy link
Collaborator

Choose a reason for hiding this comment

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

this note needs to be changed since the parameters are optional, rather, the method itself is optional.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 3c6176d

@GiulioRomualdi
Copy link
Member Author

@S-Dafarra let me know if you're fine with the modifications

@S-Dafarra
Copy link
Member

S-Dafarra commented Dec 7, 2020

@S-Dafarra let me know if you're fine with the modifications

Yes, feel free to merge as soon as the CI passes.

@GiulioRomualdi GiulioRomualdi merged commit f6dd1d6 into master Dec 7, 2020
@GiulioRomualdi GiulioRomualdi deleted the feature/are branch December 7, 2020 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants