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 test_with_logabsdet_jacobian #2

Merged
merged 9 commits into from
Oct 15, 2021
Merged

Add test_with_logabsdet_jacobian #2

merged 9 commits into from
Oct 15, 2021

Conversation

oschulz
Copy link
Collaborator

@oschulz oschulz commented Oct 13, 2021

No description provided.

Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

IMO the tests should be simplified and users should just provide a suitable Jacobian function. The implementation seems too complicated to me, and I assume it could still fail for types of x that are not covered here explicitly.

src/test.jl Outdated Show resolved Hide resolved
src/test.jl Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 13, 2021

Codecov Report

Merging #2 (cabe46d) into master (cb0d3b1) will decrease coverage by 15.55%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##            master       #2       +/-   ##
============================================
- Coverage   100.00%   84.44%   -15.56%     
============================================
  Files            1        2        +1     
  Lines           31       45       +14     
============================================
+ Hits            31       38        +7     
- Misses           0        7        +7     
Impacted Files Coverage Δ
src/with_ladj.jl 77.41% <ø> (-22.59%) ⬇️
src/test.jl 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb0d3b1...cabe46d. Read the comment docs.

@oschulz
Copy link
Collaborator Author

oschulz commented Oct 13, 2021

IMO the tests should be simplified and users should just provide a suitable Jacobian function.

Yes, simpler and more lightweight now with the optional rv_and_back argument (and this way users can just plug in functions like ParameterHandling.flatten in many cases - once that one learns to support dual numbers).

@oschulz
Copy link
Collaborator Author

oschulz commented Oct 13, 2021

Fine with you like this, @devmotion ?

@devmotion
Copy link
Member

I think it's still too confusing, I don't get what exactly one has to do even after reading the code and the docstring multiple times 😄 Can't we just drop the back stuff completely and let users just specify a function that computes the Jacobian returned by with_logabsdet_jacobian as second argument properly?

(BTW the AD vector + back functionality already exists as FiniteDifferences.to_vec: https://github.com/JuliaDiff/FiniteDifferences.jl/blob/main/src/to_vec.jl, so I don't think one should push for its support in ParameterHandling too hard)

@oschulz
Copy link
Collaborator Author

oschulz commented Oct 15, 2021

I think it's still too confusing [...] specify a function that computes the Jacobian returned by with_logabsdet_jacobian as second argument properly?

All right, you win. :-) How about now?

(BTW the AD vector + back functionality already exists as FiniteDifferences.to_vec:

Oh, right! Yes, it really think it's a fundamental thing, I think (hence JuliaGaussianProcesses/ParameterHandling.jl#43).

@oschulz
Copy link
Collaborator Author

oschulz commented Oct 15, 2021

Ok, doctests for example are in too now. Good with you, @devmotion ? If so, would release a v0.1.1 based on this and then prepare a PR on LogExpFunctions to support InverseFunctions and ChangesOfVariables.

@oschulz oschulz merged commit cabe46d into master Oct 15, 2021
@oschulz oschulz deleted the testing-routine branch October 15, 2021 13:53
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.

2 participants