-
Notifications
You must be signed in to change notification settings - Fork 317
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
Adding pseudospectral collocation schemes to Moco #3534
Conversation
Will take a look next week. |
I will take a look! |
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.
Code is looking good overall (I tried to take a careful look at all the details and indices and didn't catch any errors there). There's one (very minor) question and some documentation to address.
Reviewed 5 of 10 files at r1, 1 of 1 files at r2, 1 of 2 files at r3.
Reviewable status: 6 of 11 files reviewed, 2 unresolved discussions (waiting on @nickbianco)
OpenSim/Moco/MocoCasADiSolver/CasOCLegendreGauss.cpp
line 40 at r2 (raw file):
const int igrid = imesh * (m_degree + 1); // There are no quadrature coefficients at the mesh points (i.e., // quadCoeffs(igrid) = 0.0).
Q: does this imply that the line above DM quadCoeffs(m_numGridPoints, 1);
initializes quadCoeffs
as all 0's upon construction? (no need to update any comments or code if that's the case)
OpenSim/Moco/MocoCasADiSolver/CasOCLegendreGaussRadau.h
line 25 at r2 (raw file):
namespace CasOC { /// Enforce the differential equations in the problem using pseudospectral
these comments seem to be exactly the same as in CasOCLegendreGauss.h
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.
Reviewable status: 6 of 11 files reviewed, 2 unresolved discussions (waiting on @carmichaelong and @nickbianco)
OpenSim/Moco/MocoCasADiSolver/CasOCLegendreGauss.cpp
line 40 at r2 (raw file):
Previously, carmichaelong wrote…
Q: does this imply that the line above
DM quadCoeffs(m_numGridPoints, 1);
initializesquadCoeffs
as all 0's upon construction? (no need to update any comments or code if that's the case)
Yes, that is correct.
OpenSim/Moco/MocoCasADiSolver/CasOCLegendreGaussRadau.h
line 25 at r2 (raw file):
Previously, carmichaelong wrote…
these comments seem to be exactly the same as in
CasOCLegendreGauss.h
I've updated the comments.
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.
Thanks @carmichaelong! Ready for review.
Reviewable status: 6 of 11 files reviewed, 2 unresolved discussions (waiting on @carmichaelong and @nickbianco)
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.
Thanks! Just a few minor notes on the documentation.
Reviewed 1 of 10 files at r1, 1 of 2 files at r3, 4 of 4 files at r4.
Reviewable status: all files reviewed (commit messages unreviewed), 3 unresolved discussions (waiting on @nickbianco)
OpenSim/Moco/MocoCasADiSolver/CasOCLegendreGauss.h
line 41 at r4 (raw file):
/// mesh interval endpoint. /// /// Control approximation.
Could consider removing the first sentence here, or at least flipping the two sentences so that the details of what's implemented at least come first
OpenSim/Moco/MocoCasADiSolver/CasOCLegendreGauss.h
line 43 at r4 (raw file):
/// Control approximation. /// ---------------------- /// In [1, 2], the control is approximated using of basis of Lagrange polynomials
using a basis?
OpenSim/Moco/MocoCasADiSolver/CasOCLegendreGaussRadau.h
line 42 at r4 (raw file):
/// Control approximation. /// ---------------------- /// In [1, 2], the control is approximated using of basis of Lagrange polynomials
Same comments as in the other header:
- of -> a(?)
- consider putting what's implemented first (either by removing the first sentence or rearranging)
@carmichaelong, ready for review (just realized I never pinged you). |
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.
Reviewed 2 of 2 files at r5.
Reviewable status: all files reviewed (commit messages unreviewed), 3 unresolved discussions (waiting on @nickbianco)
This PR addes pseudospectral collocation schemes to Moco, as described in #3527.
Brief summary of changes
CasOCLegendreGauss
andCasOCLegendreGaussRadau
for the Gauss Pseudospectral Method (GPM) and Radau Pseudospectral Method (RPM), respectively. The names reflect the Legendre polynomial roots used in each scheme.MocoCasADiSolver
to make these schemes available toMocoStudy
.Testing I've completed
testMocoAnalytic
,testMocoActuators
, andtestMocoInterface
to test the new schemes.Looking for feedback on...
I'm 99% sure I implemented the schemes correctly, however I encountered some difficulty when testing the schemes on the "linear tangent steering" problem in
testMocoAnalytic
. Initially, running this test withlegendre-gauss-3
would cause the problem to almost immediately go into restoration mode in IPOPT. I updated some solver settings, including scaling problem variables, changing the CasADi finite differencing method, and reducing the number of variables (MUMPS was complaining about a memory issue at one point), and this helped with convergence.My guess is that this problem has some unique numerical difficulties that come up if you're not careful with solver settings. However, I would appreciate a second eye on the testing changes I made so we're fully confident before merging. I did write a Matlab script in CasADi to test the problem formulation, and that seems more stable than the Moco implementation -- but that problem also had the benefit of true automatic differentiation, so it's a bit hard to compare.
I'm assigning @carmichaelong to review this, but @antoinefalisse and @nicos1993, if you have some time to look over the OCP details, that would be super helpful!
CHANGELOG.md (choose one)
This change is