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

More things from Base #18

Merged
merged 3 commits into from
Feb 11, 2017
Merged

More things from Base #18

merged 3 commits into from
Feb 11, 2017

Conversation

ararslan
Copy link
Member

@ararslan ararslan commented Feb 10, 2017

This PR copies over signflip, f16, f32, and f64 from Base, and it applies @oxinabox's change from his Base PR, preserving authorship information. (I think the oxinabox + ararslan part that GitHub shows is because I have my commits set up to auto-sign, so applying the diff used Lyndon's authorship but my signoff.)

Fixes #17

@tkelman
Copy link
Contributor

tkelman commented Feb 10, 2017

lgtm

@tkelman
Copy link
Contributor

tkelman commented Feb 10, 2017

oh, f32 etc have some tests that should be moved along with the methods

src/gamma.jl Outdated
trigamma(z::Union{$T,Complex{$T}}) = $f(trigamma(f64(z)))
end
end
# TODO: better way to do this
Copy link
Contributor

@oxinabox oxinabox Feb 10, 2017

Choose a reason for hiding this comment

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

The #TODO can be removed.
I believe it was in reference not to the f64 etc.
But to the code that used that.
Which is what has been removed by my PR, and replaced with a "better way",
i,e, it is done.

Also I had (I thought) removed these definitions for f64, f32 and f16,
for the definition on line 416.
(Which is restricted so one can only cast from the known, less precise types)
And with making use of float more to find the partnered floating point type for a given integer type.
(Which works correctly for BigInts etc)

If those definitions are still required for other Special Functions,
then they probably don't belong in the gamma.jl file

@tkelman
Copy link
Contributor

tkelman commented Feb 10, 2017

f64 is still used here but f32 and f16 no longer are

@ararslan
Copy link
Member Author

Removed f32, f16, and outdated comment

@oxinabox
Copy link
Contributor

Removed f32, f16, and outdated comment

There are still 2 definitions for f64

Both work, but one is more restrictive than the other.
I personally think that for this code, where not messing up precision by mistake is important, more restrictive is better. But it is a matter of taste.
Choose one.

@ararslan
Copy link
Member Author

Sorry didn't notice the other definitions. I'll remove the old ones.

test/runtests.jl Outdated

@testset "Base Julia issue #17474" begin
@test SF.f64(complex(1f0,1f0)) == complex(1.0, 1.0)
@test SF.f64(1f0) == 1.0
Copy link
Contributor

Choose a reason for hiding this comment

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

=== to also verify the type?

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.

3 participants