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 fallbacks for log1pmx and logmxp1 #45

Merged
merged 4 commits into from
May 2, 2022

Conversation

simsurace
Copy link
Contributor

This looks like it would make sense, given that the other functions dispatch on Real argument, and it also fixes #44.

This looks like it would make sense, given that the other functions dispatch on `Real` argument, and it also fixes JuliaStats#44.
@devmotion
Copy link
Member

As I mentioned in #44, I think this PR is not the correct fix for #44, and hence the motivation is a bit different.

Regardless of that, I think it is reasonable to extend these functions to ::Real. My only concern is that the proposed implementation might not be sufficiently accurate for Float32 and Float16 for which we provide optimized implementations for other functions. It would be good to check that it is sufficiently accurate for these types, and in particular the numerically problematic inputs.

@simsurace
Copy link
Contributor Author

I see. I need to look a bit into the floating-point issues here.
I might need help from someone more familiar with those things here.
In particular, I don't really understand the optimized implementations for Float64.
I expect the problematic inputs to be at different locations for other floats.

@simsurace
Copy link
Contributor Author

simsurace commented Apr 27, 2022

Given that the PR JuliaDiff/DiffRules.jl#82 is missing this fallback, I would propose to merge this and release a new version, open a separate issue for optimizations, and add those in a future PR.

EDIT: I opened #46

@devmotion
Copy link
Member

devmotion commented Apr 27, 2022

We don't need the Float32 support in DiffRules, it's just a part of the tests that we can skip (or check that it errors).

src/basicfuns.jl Outdated Show resolved Hide resolved
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
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.

Seems fine in principle, I guess we can optimize the generic implementation in follow-up PRs if needed.

Can you add some tests with non-Float64 types (eg some Float32 values and compare their results with Float64 results) and update the version number?

@simsurace simsurace requested a review from devmotion May 2, 2022 09:47
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.

LGTM, thanks!

@devmotion devmotion merged commit 150fa44 into JuliaStats:master May 2, 2022
@simsurace simsurace deleted the patch-1 branch May 2, 2022 15:04
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
2 participants