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

fixed eltype for interval #496

Merged
merged 5 commits into from
Oct 16, 2021
Merged

Conversation

lucaferranti
Copy link
Member

@lucaferranti lucaferranti commented Oct 6, 2021

Description

changed eltype so that now eltype(1..2) returns Interval{Float64} instead of Float64. This was causing issues when doing e.g. matrix*interval multiplication.

Since a function doing what eltype was doing before can be useful, I added numtype to do that. Happy to use another name if you don't like it

Before

julia> eltype(1..2)
Float64

julia> A = [1 2;3 4]
2×2 Matrix{Int64}:
 1  2
 3  4

julia> b = 1..2
[1, 2]

julia> A*b
ERROR: MethodError: no method matching Float64(::Interval{Float64})
Closest candidates are:
  (::Type{T})(::Real, ::RoundingMode) where T<:AbstractFloat at rounding.jl:200
  (::Type{T})(::T) where T<:Number at boot.jl:760
  (::Type{T})(::AbstractChar) where T<:Union{AbstractChar, Number} at char.jl:50

After

julia> eltype(1..2)
Interval{Float64}

julia> A*b
2×2 Matrix{Interval{Float64}}:
 [1, 2]  [2, 4]
 [3, 6]  [4, 8]

Related issues

@codecov-commenter
Copy link

codecov-commenter commented Oct 6, 2021

Codecov Report

Merging #496 (7a62674) into master (7e3f494) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #496   +/-   ##
=======================================
  Coverage   91.08%   91.09%           
=======================================
  Files          25       25           
  Lines        1783     1785    +2     
=======================================
+ Hits         1624     1626    +2     
  Misses        159      159           
Impacted Files Coverage Δ
src/IntervalArithmetic.jl 100.00% <ø> (ø)
src/intervals/arithmetic.jl 97.24% <100.00%> (ø)
src/intervals/functions.jl 91.57% <100.00%> (+0.04%) ⬆️
src/intervals/hyperbolic.jl 100.00% <100.00%> (ø)
src/multidim/intervalbox.jl 89.47% <100.00%> (+0.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e3f494...7a62674. Read the comment docs.

@lbenet
Copy link
Member

lbenet commented Oct 6, 2021

Thanks @lucaferranti for addressing this! I'll try to review this PR tomorrow or over the weekend... sorry, I'm having some very busy days.

@lucaferranti
Copy link
Member Author

No problem at all Luis! Thanks for looking into this :)

@mforets
Copy link
Contributor

mforets commented Oct 11, 2021

This was causing issues when doing e.g. matrix*interval multiplication

Maybe add such test? (examples in the linked issues).

Apart from that, the proposed changes lgtm 💯

Project.toml Outdated
@@ -1,7 +1,7 @@
name = "IntervalArithmetic"
uuid = "d1acc4aa-44c8-5952-acd4-ba5d80a2a253"
repo = "https://github.com/JuliaIntervals/IntervalArithmetic.jl.git"
version = "0.19.2"
version = "0.19.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this PR introduces a potentially breaking change. (apparently, tests for eltype were lacking).

Copy link
Member

@lbenet lbenet left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for solving this. I have only added a minor comment/suggestion, which is probably not needed...

@@ -40,7 +40,7 @@ end
eltype(::Type{IntervalBox{N,T}}) where {N,T} = Interval{T} # Note that this is defined for the type


Base.eltype(x::IntervalBox{T}) where {T<:Real} = T
Base.eltype(x::IntervalBox{T}) where {T<:Real} = Interval{T}
Copy link
Member

Choose a reason for hiding this comment

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

Similar to what it's done with Interval, I guess it's a good idea to define numtype for IntervalBoxes...

Copy link
Member Author

@lucaferranti lucaferranti Oct 13, 2021

Choose a reason for hiding this comment

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

I think you are right that it can be useful, added it!

@lbenet
Copy link
Member

lbenet commented Oct 14, 2021

I propose to merge #497 first (if possible with tests for the M1 processor), then this one (it may need a rebase), and finally register the new version.

@lucaferranti
Copy link
Member Author

Agreed! It's a good idea

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.

eltype should return the full Interval type (?) Fixing broadcast requires changing eltype
4 participants