-
Notifications
You must be signed in to change notification settings - Fork 27
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
Add time derivative Jacobian tests #326
Conversation
This commit reuses the addTimeDerivative function in the multiplyWithDerivativeJacobian to avoid repetitive code
0532237
to
69422bb
Compare
The time derivative Jacobian tests did not catch a missing alpha factor in the CSTR, which was fixed in #310 We could extend the time-derivative Jacobian tests to also check for alpha. Ive also cleaned up some repetitive code in the CSTR multiplyDerivativeJacobian function, where we now use the addTimerDerivative function |
I realized that the multiplyWithDerivativeJacobian functions dont call the addTimederivativeJacobian function because we dont want to use (sparse) matrix vector multiplication here since the time derivative jacobian is even more sparse (almost a diagonal matrix). So we should probably revert the changes of commit 69422bb, even though its probably not performance relevant for the CSTR. We need a whole different, dedicated test for addTimeDerivativeJacobian |
Why not modify the old multiplyWithDerivativeJacobian function and use the matrix from addTimeDerivativeJacobian? |
because of efficieny: we dont want to use (sparse) matrix vector multiplication here since the time derivative jacobian is even more sparse (almost a diagonal matrix) |
Due to more time-sensitive tasks, we will not be working on this issue in the near future and have therefore decided to close this PR without a commit. |
No description provided.