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

Fix bugs in definitions of several Float16 methods (ref #17148). #17150

Merged
merged 1 commit into from
Jun 27, 2016

Conversation

pkofod
Copy link
Contributor

@pkofod pkofod commented Jun 27, 2016

This PR fixes the bug in

$func(a::Float16,b::Float16) = Float16($func(Float32(a),Float32(a)))
where the first positional argument is accidentally used in several Float16 methods. Fixes #17148

@vtjnash
Copy link
Member

vtjnash commented Jun 27, 2016

lgtm

@pkofod
Copy link
Contributor Author

pkofod commented Jun 27, 2016

Maybe this should even be tested as well :)
edit should I just add a test for the exact case in #17148 ? @kshyatt did you find this while working on tests for it? In that case you might have tests you want to add yourself, and I should just keep the tests added here to a minimum.

@yuyichao
Copy link
Contributor

yuyichao commented Jun 27, 2016

.... Please have a better commit message AND PR title AND PR body. None of those mention what this PR is about at all!

@kshyatt
Copy link
Contributor

kshyatt commented Jun 27, 2016

@pkofod I'm not planning on adding more tests. This LGTM but @yuyichao makes a good point as well.

@kshyatt kshyatt added the maths Mathematical functions label Jun 27, 2016
@pkofod pkofod changed the title Fix #17148 Fix bugs in definitions of several Float16 methods (ref #17148). Jun 27, 2016
@yuyichao
Copy link
Contributor

Also note that it is important to have the first line of the commit message be a summary of the change since it's what's used in shortlog and (most importantly) blame.

@pkofod
Copy link
Contributor Author

pkofod commented Jun 27, 2016

.... Please have a better commit message AND PR title AND PR body. None of those mention what this PR is about at all!

Also note that it is important to have the first line of the commit message be a summary of the change since it's what's used in shortlog and (most importantly) blame.

My bad. Used the github interface (hence the patch-2 branch name) since it was such a simple change, and might have clicked create pull request a bit too fast :) I hope it's better now.

@simonbyrne
Copy link
Contributor

LGTM: @kshyatt does this satisfy your testing requirements, or do we need to test them all?

@kshyatt
Copy link
Contributor

kshyatt commented Jun 27, 2016

This is ok with me. It's not there's some quota on PRs - we can fix it later and it's not like people were getting constantly owned by this.

@simonbyrne simonbyrne merged commit d051cb5 into JuliaLang:master Jun 27, 2016
@simonbyrne
Copy link
Contributor

Thanks!

tkelman pushed a commit that referenced this pull request Sep 13, 2016
…nt wasn't used.

Add test based on original issue #17148.

(cherry picked from commit 22a4d8b)
ref #17150
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maths Mathematical functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rem is broken for Float16
6 participants