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

Fix convert bug #320

Closed
wants to merge 3 commits into from
Closed

Fix convert bug #320

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9e2b88a
Fix convert(typeof(x), x)
dpsanders Aug 13, 2019
100eae4
Add test
dpsanders Aug 13, 2019
4be62a9
Add test of converting rational
dpsanders Aug 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/intervals/conversion.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ convert(::Type{Interval{T}}, x::Bool) where {T} = convert(Interval{T}, Int(x))
convert(::Type{Interval{T}}, x::Real) where {T} = atomic(Interval{T}, x)
convert(::Type{Interval{T}}, x::T) where {T<:Real} = Interval{T}(x)
convert(::Type{Interval{T}}, x::Interval{T}) where {T} = x
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
convert(::Type{Interval{T}}, x::Interval{T}) where {T} = x
convert(::Type{Interval{T}}, x::Interval{T}) where {T<:Real} = x

Copy link
Collaborator

Choose a reason for hiding this comment

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

While I see your point, this would make the code harder to maintain if we change the parameter type constrain.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can add a constant and use that instead of Real.

const IT = Real  # type constraint of intervals

convert(::Type{Interval{T}}, x::Interval) where {T} = atomic(Interval{T}, x)
convert(::Type{Interval{T}}, x::Interval{S}) where {T,S} = atomic(Interval{T}, x)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
convert(::Type{Interval{T}}, x::Interval{S}) where {T,S} = atomic(Interval{T}, x)
convert(::Type{Interval{T}}, x::Interval) where {T<:Real} = atomic(Interval{T}, x)


convert(::Type{Interval}, x::Real) = (T = typeof(float(x)); convert(Interval{T}, x))
convert(::Type{Interval}, x::Interval) = x
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not directly related to the issue, but this definition is the same as the one on line 20 and should probably be deleted (especially since the result would not be correct if for some weird reason it ever gets called).

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I don't understand which definition you're talking about that is the same as the one on line 20?

Copy link
Collaborator

Choose a reason for hiding this comment

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

On line 23:

convert(::Type{Interval}, x::Interval) = x

Since Interval is the same as Interval{T} where {T} and nothing there requests both parameter to be the same.

Expand Down
15 changes: 15 additions & 0 deletions test/interval_tests/construction.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ using Test

const eeuler = Base.MathConstants.e

using InteractiveUtils


@testset "Constructing intervals" begin
setprecision(Interval, 53)
Expand Down Expand Up @@ -340,3 +342,16 @@ import IntervalArithmetic: force_interval
@test force_interval(Inf, -Inf) == Interval(-Inf, Inf)
@test_throws ArgumentError force_interval(NaN, 3)
end

@testset "#320 fixing convert bug" begin
x = 3..4
T = typeof(x)

m = @which convert(T, x)

@test m.sig == Tuple{typeof(convert),Type{Interval{T}},Interval{T}} where T

x = interval(1//1, 5//3)
@test convert(typeof(x), x) == x
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@test convert(typeof(x), x) == x
@test convert(typeof(x), x) === x

This would check that it does not create a new interval.


end