-
Notifications
You must be signed in to change notification settings - Fork 53
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
Minimal changes to jacobian in calculus.jl to allow for nonsquare Jacobians #372
Conversation
oh and this resolves #371 |
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.
Thanks for your contribution. I left essentially one suggestion (four times)... Aside from that, LGTM!
src/calculus.jl
Outdated
|
||
@inbounds for comp = 1:numVars | ||
@inbounds for comp = 1:length(vf) |
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.
I think the following is recommended (it should have been changed before, but ...):
@inbounds for comp = 1:length(vf) | |
@inbounds for comp in eachindex(vf) |
src/calculus.jl
Outdated
|
||
for comp = 1:numVars | ||
for comp = 1:length(vf) |
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.
Same suggestion as above...
src/calculus.jl
Outdated
for comp2 = 1:numVars | ||
for comp1 = 1:numVars | ||
for comp1 = 1:length(vf) |
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.
Same suggestion, again
src/calculus.jl
Outdated
for comp = 1:numVars | ||
@inbounds for nv = 1:numVars | ||
@inbounds for nv = 1:length(vf) |
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.
Same suggestion again
All changes implemented now |
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.
LGTM. Merging!
I enabled the CI workflow on my clone and things seemed OK, I hope there are some tests being run there becase I did not run any checks/tests manually :D