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 diffrules for log1pmx and logmxp1 #82

Merged
merged 8 commits into from
Apr 28, 2022

Conversation

simsurace
Copy link
Contributor

@simsurace simsurace commented Apr 27, 2022

This is supposed to fix JuliaStats/LogExpFunctions.jl#44.

There may be more optimal orders of operations for these derivatives.
Also, could someone assist me in what would be mandatory tests for these additions?

@simsurace
Copy link
Contributor Author

Looks like JuliaStats/LogExpFunctions.jl#45 is actually a prerequisite.

@devmotion
Copy link
Member

It's already tested automatically but maybe it's helpful to adjust the domain of inputs (both for the primal and the finite diff comparisons), similar to some other functions in runtests.jl. I don't remember the default domain by heart, so maybe it's not needed at all.

Ironically, tests fail because the functions are not defined for Float32 (yet). So for the time being, probably we should skip Float32 tests for these inputs (or check that they throw an error, to be notified when this changes).

@codecov-commenter
Copy link

codecov-commenter commented Apr 27, 2022

Codecov Report

Merging #82 (b1795d3) into master (7dde134) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #82      +/-   ##
==========================================
+ Coverage   96.96%   97.00%   +0.03%     
==========================================
  Files           3        3              
  Lines         165      167       +2     
==========================================
+ Hits          160      162       +2     
  Misses          5        5              
Impacted Files Coverage Δ
src/rules.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 7dde134...b1795d3. Read the comment docs.

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.

Thanks, looks good! I just have one suggestion to make finite differencing even less likely to fail.

src/rules.jl Outdated Show resolved Hide resolved
test/runtests.jl Show resolved Hide resolved
@simsurace simsurace requested a review from devmotion April 28, 2022 12:58
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.

Looks good to me, thank you!

I'll merge and release once tests pass (and feel free to ping me if I forget it 🙂).

@devmotion
Copy link
Member

The MTK test errors are unrelated.

@devmotion devmotion merged commit ad53ba3 into JuliaDiff:master Apr 28, 2022
@simsurace simsurace deleted the logexpfuns_additions branch April 28, 2022 14:13
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.

log1pmx and logmxp1 cannot be differentiated by ForwardDiff
3 participants