-
Notifications
You must be signed in to change notification settings - Fork 679
[wpimath] Implement Tsitouras 5th order numerical integrator #7383
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: main
Are you sure you want to change the base?
Conversation
7a33067 to
17135d5
Compare
wpimath/src/main/native/include/frc/system/NumericalIntegration.h
Outdated
Show resolved
Hide resolved
wpimath/src/test/java/edu/wpi/first/math/system/NumericalIntegrationTest.java
Outdated
Show resolved
Hide resolved
wpimath/src/main/native/include/frc/system/NumericalIntegration.h
Outdated
Show resolved
Hide resolved
wpimath/src/main/native/include/frc/system/NumericalIntegration.h
Outdated
Show resolved
Hide resolved
wpimath/src/main/java/edu/wpi/first/math/system/NumericalIntegration.java
Outdated
Show resolved
Hide resolved
wpimath/src/main/native/include/frc/system/NumericalIntegration.h
Outdated
Show resolved
Hide resolved
wpimath/src/main/native/include/frc/system/NumericalIntegration.h
Outdated
Show resolved
Hide resolved
wpimath/src/main/java/edu/wpi/first/math/system/NumericalIntegration.java
Outdated
Show resolved
Hide resolved
wpimath/src/test/native/cpp/system/NumericalIntegrationTest.cpp
Outdated
Show resolved
Hide resolved
8d64dab to
f39bda2
Compare
|
/format |
wpimath/src/test/native/cpp/system/NumericalIntegrationTest.cpp
Outdated
Show resolved
Hide resolved
calcmogul
left a comment
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.
You forgot the WPI_UNIGNORE_DEPRECATED macro that pops the diagnostic state.
wpimath/src/test/native/cpp/system/NumericalIntegrationTest.cpp
Outdated
Show resolved
Hide resolved
|
Looks like the arm simulation test hung in C++ and Java on a few platforms. This could indicate a bug in the new integrator that's causing the max error to be really large (so the steps are tiny and the solver takes forever). |
|
The issue could be related to numerical precision, a bug in the implementation, or a bug in the paper. You could compare it against https://github.com/SciML/OrdinaryDiffEq.jl/tree/master/lib/OrdinaryDiffEqTsit5/src. |
|
The float method in the linked repo appeared to be at most 4 decimal places more precise, I can try updating the constants. In addition, the signs for the b2 constants are flipped in my code and the paper when compared to the other repo, so I will try swapping those as well. |
|
Realized it was just because instead of using b2-b1, the error was supposed to use b2. |
Update wpimath/src/test/native/cpp/system/NumericalIntegrationTest.cpp Fix deprecation suppression Update NumericalIntegration.java Update NumericalIntegration.java Co-Authored-By: Tyler Veness <calcmogul@gmail.com>
Are you sure? https://en.wikipedia.org/wiki/Dormand%E2%80%93Prince_method says b1 gives the 5th order solution and b2 gives the 4th order solution, and the error tolerance is the difference between the two outputs.
Section 1 of the Tsitouras paper says the same thing:
|
|
@calcmogul The way I used was the way the link you sent me does it; the b1 values are all commented out and unused, instead only btilde (which are the b2 values) are used. Perhaps I'm wrong, but I ran the tests locally and everything checked out. |
|
So does that mean the paper is wrong? |
|
I am not sure, I need to look deeper into the other repository to see why b1 was commented out to have a clearer answer. Obviously, the build checks failing are indicating that there is some other issue at play on the c++ side, however swapping to using purely the b2 values did allow the testTsit5TimeVarying function to run quickly for me in Java when it couldn't previously. |
|
Looks like the java errors are from the unit test not having enough tolerance(just barely), and the c++ errors should theoretically be fixed by updating some of the constants I missed? I'll test some more before making another push. |
|
Since a₇,ᵢ = bᵢ, it seems like the linked repository uses the |
Closes #7123