Skip to content

Conversation

@dlfivefifty
Copy link
Contributor

Still need to change elsewhere, this is just a reminder to rename vector_mode_dual_eval to make it clear it mutates its inputs (was confusing when debugging).

@KristofferC
Copy link
Collaborator

Bump

@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2021

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.88%. Comparing base (6b393f4) to head (010756b).
⚠️ Report is 99 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #528      +/-   ##
==========================================
+ Coverage   84.83%   84.88%   +0.04%     
==========================================
  Files           9        9              
  Lines         831      827       -4     
==========================================
- Hits          705      702       -3     
+ Misses        126      125       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dlfivefifty
Copy link
Contributor Author

The 1.0 failure seems unrelated

@KristofferC
Copy link
Collaborator

KristofferC commented Jul 28, 2021

I guess you would kind of think that this modifies x and not cfg. Perhaps the argument order should be swapped? What do you think?

@dlfivefifty
Copy link
Contributor Author

Agreed (though only subject to the "kind of" qualifier... I think anything that mutates any of the arguments should use !).

I'll change it

src/jacobian.jl Outdated

function vector_mode_jacobian(f::F, x, cfg::JacobianConfig{T,V,N}) where {F,T,V,N}
ydual = vector_mode_dual_eval!(f, x, cfg)
ydual = vector_mode_dual_eval!(cfg, f, x)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even in ! functions I think you put the function first to allow for do syntax. So it would be f, cfg, x.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are really trying to make me regret this...

ok I've changed it again

@dlfivefifty
Copy link
Contributor Author

Hmm now the failure is on Julia 1, but also seems unrelated to this PR...

@KristofferC
Copy link
Collaborator

Yeah, there is some numerical thing we need to investigate at some point..

@KristofferC KristofferC merged commit c6dc209 into JuliaDiff:master Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants