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

Added zero, iszero, one, isone to dual #74

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

adamnemecek
Copy link

No description provided.

@ararslan
Copy link
Member

Cool, thanks for the contribution. Could you add tests?

src/dual.jl Outdated
@@ -189,6 +189,17 @@ Base.ceil( ::Type{T}, z::Dual) where {T<:Real} = ceil( T, value(z))
Base.trunc(::Type{T}, z::Dual) where {T<:Real} = trunc(T, value(z))
Base.round(::Type{T}, z::Dual) where {T<:Real} = round(T, value(z))

Base.zero(::Type{Dual{T}}) where {T} = Dual(zero(T), zero(T))
Base.zero(x::Dual{T}) where {T} = zero(T)
Base.iszero(z::Dual{T}) where {T} = iszero(value(z)) & iszero(epsilon(z))
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason not to use short-circuiting && here and for isone?

src/dual.jl Outdated
@@ -189,6 +189,17 @@ Base.ceil( ::Type{T}, z::Dual) where {T<:Real} = ceil( T, value(z))
Base.trunc(::Type{T}, z::Dual) where {T<:Real} = trunc(T, value(z))
Base.round(::Type{T}, z::Dual) where {T<:Real} = round(T, value(z))

Base.zero(::Type{Dual{T}}) where {T} = Dual(zero(T), zero(T))
Base.zero(x::Dual{T}) where {T} = zero(T)
Base.iszero(z::Dual{T}) where {T} = iszero(value(z)) & iszero(epsilon(z))
Copy link
Contributor

Choose a reason for hiding this comment

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

This definition of iszero is inconsistent with how == 0 is defined for duals:

Base.:(==)(z::Dual, x::Number) = value(z) == x

src/dual.jl Outdated
@@ -189,6 +189,17 @@ Base.ceil( ::Type{T}, z::Dual) where {T<:Real} = ceil( T, value(z))
Base.trunc(::Type{T}, z::Dual) where {T<:Real} = trunc(T, value(z))
Base.round(::Type{T}, z::Dual) where {T<:Real} = round(T, value(z))

Base.zero(::Type{Dual{T}}) where {T} = Dual(zero(T), zero(T))
Base.zero(x::Dual{T}) where {T} = zero(T)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why should these two versions of zero return different types?

@@ -189,6 +189,21 @@ Base.ceil( ::Type{T}, z::Dual) where {T<:Real} = ceil( T, value(z))
Base.trunc(::Type{T}, z::Dual) where {T<:Real} = trunc(T, value(z))
Base.round(::Type{T}, z::Dual) where {T<:Real} = round(T, value(z))

Base.zero(::Type{Dual{T}}) where {T} = Dual(zero(T), zero(T))
Base.zero(x::Dual{T}) where {T} = zero(typeof(x))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not

Suggested change
Base.zero(x::Dual{T}) where {T} = zero(typeof(x))
Base.zero(::Dual{T}) where {T} = zero(Dual{T})

src/dual.jl Outdated Show resolved Hide resolved
Copy link
Contributor

@briochemc briochemc left a comment

Choose a reason for hiding this comment

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

A micro suggestion 🙂

@@ -3,6 +3,7 @@ using Test
using LinearAlgebra
import DualNumbers: value
import NaNMath
import Base: one, isone, zero, iszero
Copy link
Member

Choose a reason for hiding this comment

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

These are exported from base, so this is unnecessary.

Suggested change
import Base: one, isone, zero, iszero

Copy link
Author

Choose a reason for hiding this comment

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

You need to explicitly import it to extend it. Try this.

struct ABC end
one(x::ABC) = 1

Will give you the error

ERROR: error in method definition: function Base.one must be explicitly imported to be extended
Stacktrace:
[1] top-level scope at none:0

Copy link
Member

Choose a reason for hiding this comment

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

I know that, but they are not being extended in the tests.

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.

4 participants