Make has_original_jac check original_jac #862
Conversation
It seems to me that this makes more sense as a definition. And that doing it this way, makes my code work. (though I haven't checked correctness) Whereas without this, the branch is hit that tries to compute the original jacobian via finite differencing (with `autodiff=false, autojacvec=false, autojacmat=false`)
![ai-maintainer[bot]](https://avatars.githubusercontent.com/in/339956?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI-Maintainer Review for PR - Make has_original_jac check original_jac
Title and Description 👍
The title and description provide a general understanding of the changes
The title and description of the pull request give a basic understanding of the changes. They indicate that the has_original_jac function has been updated to check S.original_jac instead of S.jac. However, it would be beneficial to provide more context and detail about why this change is necessary and what problem it solves.
Scope of Changes 👍
The changes are narrowly focused
The changes in this pull request are narrowly focused on a specific issue. The diff shows a modification to the has_original_jac function, updating its definition to check S.original_jac instead of S.jac. There is no evidence of the author trying to resolve multiple issues simultaneously.
Testing 👎
Testing details are missing
The description does not provide any information about how the changes were tested. It would be beneficial for you to include details about the testing approach you used, such as specific test cases or scenarios you considered, to ensure the changes are functioning as intended.
Suggested Changes
- Please provide more context in the PR description about why this change is necessary and what problem it solves.
- Include details about how you tested the changes. This could be specific test cases or scenarios you considered.
Thank you for your contribution!
Reviewed with AI Maintainer
|
Can you add a test for this? |
| prob = ODEForwardSensitivityProblem(f, [1.0; 1.0], (0.0, 10.0), p, absolutely_no_ad_sensealg) | ||
| @test SciMLSensitivity.has_original_jac(prob.f) | ||
| @assert jac_call_count == 0 | ||
| solve(prob, Tsit5(), abstol = 1e-14, reltol = 1e-14) |
There was a problem hiding this comment.
This actually errors, without this change.
Which I think might be an unrelated bug. Not sure
|
tests failing seem to be unrelated. |
|
bump |
| absolutely_no_ad_sensealg = ForwardSensitivity(autodiff = false, | ||
| autojacvec = false, | ||
| autojacmat = false) | ||
| prob = ODEForwardSensitivityProblem(f, | ||
| [1.0; 1.0], | ||
| (0.0, 10.0), | ||
| p, | ||
| absolutely_no_ad_sensealg) |
There was a problem hiding this comment.
Shit I didn't check this. This looks to be the wrong format.
There was a problem hiding this comment.
I just used julia formatter with the SciMLStyle
It seems to me that this makes more sense as a definition. And that doing it this way, makes my code work.
(though I haven't checked correctness)
Whereas without this, the branch is hit that tries to compute the original jacobian via finite differencing (with
autodiff=false, autojacvec=false, autojacmat=false)