- 
        Couldn't load subscription status. 
- Fork 665
[wpimath] Add Sleipnir Java bindings #8236
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: 2027
Are you sure you want to change the base?
[wpimath] Add Sleipnir Java bindings #8236
Conversation
7e667cd    to
    9749bdc      
    Compare
  
    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.
Still haven't gotten to the tests, but the source looks pretty good.
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.
Given the size of this PR, it'd be nice if the sleipnir update had it's own PR so that its changes wouldn't be buried.
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.
Yea, I was mainly bumping it here to keep it in sync with upstream bugfixes. I'll make a separate PR once review is done and the bugfixes slow down.
437d2e1    to
    ef56a07      
    Compare
  
    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.
Still reviewing the other tests, but submitting some initial comments now so that those can get addressed.
On another note, the tests leak every single Variable (including intermediates) since they are never closed. Unfortunately, I don't think we can really do anything about that (at least, not on the test side).
        
          
                wpimath/src/test/java/edu/wpi/first/math/autodiff/GradientTest.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      ef56a07    to
    d771285      
    Compare
  
    | Yea... I was hoping the GC called close(), but it seems not. In any case, manually closing them or using try-with-resources with all of them is too onerous. Java deprecated and removed finalizers in favor of try-with-resource and cleaners, but idk if cleaners are much better. | 
d771285    to
    b046228      
    Compare
  
    b046228    to
    bc2b157      
    Compare
  
    51fec82    to
    cc8b7a9      
    Compare
  
    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.
Done reviewing everything now (though a few parts were more verifying that it matched the original Sleipnir, not necessarily verifying correctness).
        
          
                wpimath/src/test/java/edu/wpi/first/math/autodiff/HessianTest.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                wpimath/src/test/java/edu/wpi/first/math/autodiff/HessianTest.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                wpimath/src/test/java/edu/wpi/first/math/optimization/NonlinearProblemTest.java
          
            Show resolved
            Hide resolved
        
              
          
                wpimath/src/test/java/edu/wpi/first/math/optimization/FlywheelOCPTest.java
          
            Show resolved
            Hide resolved
        
              
          
                wpimath/src/test/java/edu/wpi/first/math/autodiff/VariableMatrixTest.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                wpimath/src/test/java/edu/wpi/first/math/autodiff/VariableMatrixTest.java
          
            Show resolved
            Hide resolved
        
              
          
                wpimath/src/test/java/edu/wpi/first/math/autodiff/VariableMatrixTest.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                wpimath/src/test/java/edu/wpi/first/math/autodiff/VariableMatrixTest.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      8b5e065    to
    0f442e5      
    Compare
  
    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.
Seems fine to me for what it's worth, except some lingering concerns about the difference in accuracy of FlywheelOCP.
5ba0fc9    to
    41e3f36      
    Compare
  
            
          
                wpimath/src/test/java/edu/wpi/first/math/optimization/DoubleIntegratorProblemTest.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      681ee08    to
    adfab84      
    Compare
  
    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.
Now that the FlywheelOCP stuff is sorted out, seems fine to me for what it's worth.
adfab84    to
    5c2e6d0      
    Compare
  
    
The wrapper includes reverse mode autodiff, the Problem DSL, and the optimal control problem API. I wrote it by directly translating the upstream API and tests to Java (i.e., copy-paste-modify).
I replaced the ArmFeedforward and Ellipse2d JNIs with implementations using the Sleipnir Java bindings. Switching dev binary JNIs to release by default sped up wpimath test runs from several minutes to 7 seconds.