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.

Sign up for GitHub

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

Jump to bottom

Add iszero(x) branches to xlogy and xlog1py #57

Merged
merged 11 commits into from
Aug 23, 2022
Merged

Add iszero(x) branches to xlogy and xlog1py #57

merged 11 commits into from
Aug 23, 2022

Conversation

simsurace
Copy link
Contributor

@simsurace simsurace commented Aug 19, 2022 •
edited
Loading

Apologies for the new PR, but I accidentally deleted my remote branch. This replaces #54

devmotion
devmotion reviewed Aug 19, 2022
View reviewed changes
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.

Hmm it seems there are still some test failures.

@simsurace
Copy link
Contributor Author

Hmm I saw them pass locally. Will get back to this...

simsurace and others added 4 commits August 22, 2022 10:51
@simsurace @devmotion
White space
541afd9 
Co-authored-by: David Widmann <[email protected]>
@simsurace @devmotion
White space
69897d0 
Co-authored-by: David Widmann <[email protected]>
@simsurace @devmotion
Simplify expression
cb06456 
Co-authored-by: David Widmann <[email protected]>
@simsurace @devmotion
Improve formatting
d65d9fa 
Co-authored-by: David Widmann <[email protected]>
devmotion
devmotion reviewed Aug 22, 2022
View reviewed changes
@simsurace
Copy link
Contributor Author

I needed to modify the frule in order to make this pass. Since the derivative wrt. x is infinite, the finite difference tests would fail. I therefore replaced the test_frule and test_rrule by manual tests of just the y derivative.

@simsurace
Copy link
Contributor Author

I modified the wrong frule, apologies.

@simsurace simsurace requested a review from devmotion August 22, 2022 09:37
devmotion
devmotion reviewed Aug 22, 2022
View reviewed changes
@simsurace simsurace requested a review from devmotion August 23, 2022 08:22
devmotion
devmotion approved these changes Aug 23, 2022
View reviewed changes
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.

OK, let's get this in! It's definitely an improvement and tests pass finally 🙂 We can always add more branches later if needed.

@devmotion devmotion merged commit c90f261 into JuliaStats:master Aug 23, 2022
@simsurace simsurace deleted the xlogy_etc branch August 23, 2022 09:21
@simsurace simsurace mentioned this pull request Aug 24, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devmotion devmotion devmotion approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@simsurace @devmotion