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

Add time derivative Jacobian tests #326

Closed
wants to merge 1 commit into from

Conversation

AntoniaBerger
Copy link
Collaborator

No description provided.

@AntoniaBerger AntoniaBerger changed the title fix/CSTR time derivative jacobian test Fix CSTR time derivative jacobian test Nov 22, 2024
This commit reuses the addTimeDerivative function in the
multiplyWithDerivativeJacobian to avoid repetitive code
@jbreue16 jbreue16 force-pushed the test/add_time_derivative_test branch from 0532237 to 69422bb Compare November 22, 2024 10:45
@jbreue16
Copy link
Contributor

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.
It might seem trivial since we would check for $\alpha \cdot J$ instead of just $J$, but since this already lead to an error we've decided to catch this.
We cannot use AD for this check since AD units use the same addTimeDerivative function for that part of the Jacobian.
So we need to adapt the FD Jacobian check for $\alpha != 1.0$

Ive also cleaned up some repetitive code in the CSTR multiplyDerivativeJacobian function, where we now use the addTimerDerivative function

@jbreue16
Copy link
Contributor

jbreue16 commented Nov 22, 2024

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

@jbreue16 jbreue16 changed the title Fix CSTR time derivative jacobian test Add time derivative Jacobian test Nov 22, 2024
@jbreue16 jbreue16 changed the title Add time derivative Jacobian test Add time derivative Jacobian tests Nov 22, 2024
@jbreue16 jbreue16 added the test label Nov 22, 2024
@AntoniaBerger
Copy link
Collaborator Author

Why not modify the old multiplyWithDerivativeJacobian function and use the matrix from addTimeDerivativeJacobian?

@jbreue16
Copy link
Contributor

jbreue16 commented Nov 22, 2024

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)

@AntoniaBerger
Copy link
Collaborator Author

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.
To keep track of this problem see issue #330

@github-actions github-actions bot locked and limited conversation to collaborators Nov 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants