RFC: tests for dimensional correctness of Base #20484

stevengj · 2017-02-07T03:48:41Z

This PR (currently very incomplete) adds a test suite dimensionful.jl that implements a small dimensionful number type Furlong that can be used to test the dimensional correctness of Base functions.

I already caught an bug in range, and there are probably lots more problems.

cc @timholy

timholy · 2017-02-07T05:29:50Z

CC @ajkeller34

ajkeller34 · 2017-02-07T06:52:35Z

I know some of the LU factorization code is dimensionally inconsistent, and I'd love for Unitful to work better with linear algebra. See Unitful #46. I actually have a local branch of Unitful where I was playing around with how far I could get without changing Base, and I think I got stuck somewhere, so I will try to recall what the problem was.

Edit: Of course, I recognize some of this may not be trivial, and it may be better to do some prototyping in a package, which is sort of what I had in mind originally. I just thought I would mention it while the topic is being discussed.

andreasnoack · 2017-02-07T11:59:45Z

@ajkeller34 We discussed this long time ago in #7623. As such, the algorithm used in generic_lufact! can handle units but it fails because it tries to store the unitless L in the input matrix. In the tradition of LAPACK, all of our dense linear algebra reuses input memory to store the result and that is not possible for tightly types unitful matrices. If you convert the element type of matrix to an abstract type then the factorization succeeds

julia> a = [1.0u"N" 2.0u"N"
             3.0u"N" 1.0u"N"]
2×2 Array{Quantity{Float64, Dimensions:{𝐋 𝐌 𝐓^-2}, Units:{N}},2}:
 1.0 N  2.0 N
 3.0 N  1.0 N

julia> lufact!(Matrix{Number}(a), Val{false})
Base.LinAlg.LU{Number,Array{Number,2}}(Number[1.0 N 2.0 N; 3.0 -5.0 N],[1,2],0)

but this is not really satisfying. We could try experimenting with widening of the type as the factorization progresses similarly to broadcast!.

timholy · 2017-02-07T12:58:54Z

For Cholesky factorization there's a good argument that L should have sqrt(units(A)), but I can't really see that's true of LU factorization. To me it seems perfectly fine to have lufact(A) fail for unitful quantities, but to support lufact!(dest, A) where dest is a suitably-preallocated object.

GlenHertz

Looks like a few typos

GlenHertz · 2017-02-08T00:34:52Z

test/dimensionful.jl

+Base.abs{p,T}(x::Furlong{p,T}) = Furlong{p,T}(abs(x.val))
+
+for f in (:isfinite, :isnan, :isreal)
+    @eval Base.$f(x::Furlong) = isfinite(x.val)


Should be @eval Base.$f(x::Furlong) = $f(x.val)?

GlenHertz · 2017-02-08T00:35:16Z

test/dimensionful.jl

+    @eval Base.$f(x::Furlong) = isfinite(x.val)
+end
+for f in (:real,:imag,:complex)
+    @eval Base.$f{p,T}(x::Furlong{p,T}) = Furlong{p}(f(x.val))


Should be @eval Base.$f{p,T}(x::Furlong{p,T}) = Furlong{p}($f(x.val))?

stevengj · 2017-02-08T04:03:26Z

Hmm, found another problem: the StepRange{T,S} iteration code assumes that start::T + step::S can be converted to type T, but this is not guaranteed by the constructor. In particular, we define colon{T}(start::T, step, stop::T) = StepRange(start, step, stop), but it seems like this should be:

function colon{T}(start::T, step, stop::T)
    T′ = typeof(start+step)
    StepRange(convert(T′,start), step, convert(T′,stop))
end

I'm not really sure why StepRange allows the step to be of a different type than the endpoints at all. @StefanKarpinski, can you comment on this?

stevengj · 2017-02-08T04:13:45Z

Hmm, for dimensionful quantities it looks like it would be nice to have isnegative(x) to use instead of x < 0. Of course, you can define <(x::Furlong, y::Real) to work if y==0 and throw an error otherwise, but it is a bit ugly.

JeffBezanson · 2017-02-08T04:14:06Z

That case handles ordinal types, e.g. 'a':2:'z'.

TotalVerb · 2017-02-08T04:46:11Z

Another case is with Date and Period.

stevengj · 2017-02-08T04:49:41Z

I fixed a couple more dimensional problems. My inclination would be to merge these tests and fixes now, assuming tests are green, and put additional fixes in subsequent PRs.

timholy · 2017-02-08T13:52:46Z

Hmm, for dimensionful quantities it looks like it would be nice to have isnegative(x) to use instead of x < 0.

What's wrong with x < zero(x)? I'd rather not proliferate too many names...

timholy

See issues regarding ranges. I wonder if it would be better to move the code that defines Furlong and its operations into TestHelpers?

timholy · 2017-02-08T14:01:04Z

base/range.jl

@@ -6,9 +6,9 @@ colon{T<:Real}(start::T, stop::T) = UnitRange{T}(start, stop)

 range(a::Real, len::Integer) = UnitRange{typeof(a)}(a, oftype(a, a+len-1))

-colon{T}(start::T, stop::T) = colon(start, oftype(stop-start, 1), stop)
+colon{T}(start::T, stop::T) = colon(start, oneunit(stop-start), stop)


Sorry I didn't look at this earlier. I think the original version was correct, and that this version is a bug. This has been discussed elsewhere and there isn't a uniform opinion, but I would find the following worrisome:

xmm, ymm = 1000mm, 2000mm xm, ym = convert(Meter, xmm), convert(Meter, ymm) xmm == xm && ymm == ym => true (xmm:ymm) == (xm:ym) => false

I think it's a bad idea to allow start:stop constructors for unitful quantities; one should be required to specify the step explicitly. The oftype version of this code forces that.

timholy · 2017-02-08T14:02:18Z

base/range.jl

@@ -46,7 +49,7 @@ _range{T,S}(::Any, ::Any, a::T, step::S, len::Integer) = StepRangeLen{typeof(a+0

 # AbstractFloat specializations
 colon{T<:AbstractFloat}(a::T, b::T) = colon(a, T(1), b)
-range(a::AbstractFloat, len::Integer) = range(a, oftype(a, 1), len)
+range(a::AbstractFloat, len::Integer) = range(a, oneunit(a), len)


timholy · 2017-02-08T14:03:19Z

test/dimensionful.jl

+end
+Base.sqrt{p,T}(x::Furlong{p,T}) = _div(sqrt(x.val), x, Val{2})
+
+@test collect(Furlong(2):Furlong(10)) == collect(Furlong(2):Furlong(1):Furlong(10)) == Furlong.(2:10)


Turn the first of these into a @test_throws

StefanKarpinski · 2017-02-08T14:29:53Z

What's wrong with x < zero(x)? I'd rather not proliferate too many names...

x has units and zero(x) does not.

timholy · 2017-02-08T15:03:59Z

Why would the additive identity not have units?

julia> using Unitful

julia> using Unitful.m

julia> x = 1m
1 m

julia> zero(x)
0 m

julia> x + zero(x)
1 m

StefanKarpinski · 2017-02-08T17:44:31Z

Oh right, my mistake! 🥇

stevengj · 2017-02-08T19:31:38Z

The only problem with x < zero(x) is that it may be inefficient for something like BigFloat, since zero(x) has to allocate a new object, and comparisons with Int64 are faster.

stevengj · 2017-02-08T19:32:14Z

But probably if you are doing the sqrtm of a BigFloat matrix the comparison with zero is the least of your worries. I'll change it.

stevengj · 2017-02-10T02:40:01Z

Rebased. Good to squash/merge when green?

…d canonicalize exponents

…in TestHelpers and split tests into the relevant files, use x < zero(x) rather than x < 0 in sqrtm

stevengj · 2017-02-11T19:59:14Z

Rebased again.

tkelman · 2017-02-14T12:55:19Z

this has caused the ambiguity test to start segfaulting. happens often on the buildbots, and it's reproducible locally with make testall1

stevengj added the test This change adds or pertains to unit tests label Feb 7, 2017

StefanKarpinski approved these changes Feb 7, 2017

View reviewed changes

GlenHertz reviewed Feb 8, 2017

View reviewed changes

timholy reviewed Feb 8, 2017

View reviewed changes

stevengj mentioned this pull request Feb 8, 2017

parse x^literal as call to ^(x, Val{literal})? #20527

Closed

stevengj changed the title ~~WIP: tests for dimensional correctness of Base~~ RFC: tests for dimensional correctness of Base Feb 8, 2017

stevengj force-pushed the dimensionful branch 2 times, most recently from df10330 to 8871684 Compare February 10, 2017 02:39

timholy approved these changes Feb 10, 2017

View reviewed changes

stevengj added 3 commits February 11, 2017 14:57

tests for dimensional correctness of Base

114230b

fix typos

fc86860

another test and a missing promotion

19bf25b

stevengj and others added 9 commits February 11, 2017 14:57

missing promotion for colon(start, step, stop)

686ad93

test for missing colon promotion, add a couple of missing methods, an…

89bbbf1

…d canonicalize exponents

fix sqrtm for dimensionful

06fa77d

stats dimensionful tests, fixed units of momenttype

d872d1d

revert changes to range for Furlong(1):Furlong(10), put Furlong type …

988b109

…in TestHelpers and split tests into the relevant files, use x < zero(x) rather than x < 0 in sqrtm

dimensionful is no longer a separate test suite

2b5b03b

undo change to range(a,len) constructor and add a test

b1bd099

added missing Furlong constructor and fix test typo

b282c29

new inner constructor syntax ala JuliaLang#20308

466dc70

stevengj force-pushed the dimensionful branch from 8871684 to 466dc70 Compare February 11, 2017 19:58

stevengj merged commit 43e2aad into JuliaLang:master Feb 11, 2017

stevengj deleted the dimensionful branch February 11, 2017 23:05

musm mentioned this pull request Apr 26, 2017

speed-up prevpow2/nextpow2 #21231

Merged

briochemc mentioned this pull request Jun 3, 2019

Unitful linear algebra PainterQubits/Unitful.jl#46

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: tests for dimensional correctness of Base #20484

RFC: tests for dimensional correctness of Base #20484

stevengj commented Feb 7, 2017

timholy commented Feb 7, 2017

ajkeller34 commented Feb 7, 2017 •

edited

Loading

andreasnoack commented Feb 7, 2017

timholy commented Feb 7, 2017

GlenHertz left a comment

GlenHertz Feb 8, 2017 •

edited

Loading

stevengj Feb 8, 2017

GlenHertz Feb 8, 2017 •

edited

Loading

stevengj Feb 8, 2017

stevengj commented Feb 8, 2017

stevengj commented Feb 8, 2017

JeffBezanson commented Feb 8, 2017

TotalVerb commented Feb 8, 2017

stevengj commented Feb 8, 2017

timholy commented Feb 8, 2017

timholy left a comment

timholy Feb 8, 2017

stevengj Feb 8, 2017

timholy Feb 8, 2017

timholy Feb 8, 2017

StefanKarpinski commented Feb 8, 2017

timholy commented Feb 8, 2017

StefanKarpinski commented Feb 8, 2017

stevengj commented Feb 8, 2017

stevengj commented Feb 8, 2017

stevengj commented Feb 10, 2017 •

edited

Loading

stevengj commented Feb 11, 2017

tkelman commented Feb 14, 2017 •

edited

Loading

RFC: tests for dimensional correctness of Base #20484

RFC: tests for dimensional correctness of Base #20484

Conversation

stevengj commented Feb 7, 2017

timholy commented Feb 7, 2017

ajkeller34 commented Feb 7, 2017 • edited Loading

andreasnoack commented Feb 7, 2017

timholy commented Feb 7, 2017

GlenHertz left a comment

Choose a reason for hiding this comment

GlenHertz Feb 8, 2017 • edited Loading

Choose a reason for hiding this comment

stevengj Feb 8, 2017

Choose a reason for hiding this comment

GlenHertz Feb 8, 2017 • edited Loading

Choose a reason for hiding this comment

stevengj Feb 8, 2017

Choose a reason for hiding this comment

stevengj commented Feb 8, 2017

stevengj commented Feb 8, 2017

JeffBezanson commented Feb 8, 2017

TotalVerb commented Feb 8, 2017

stevengj commented Feb 8, 2017

timholy commented Feb 8, 2017

timholy left a comment

Choose a reason for hiding this comment

timholy Feb 8, 2017

Choose a reason for hiding this comment

stevengj Feb 8, 2017

Choose a reason for hiding this comment

timholy Feb 8, 2017

Choose a reason for hiding this comment

timholy Feb 8, 2017

Choose a reason for hiding this comment

StefanKarpinski commented Feb 8, 2017

timholy commented Feb 8, 2017

StefanKarpinski commented Feb 8, 2017

stevengj commented Feb 8, 2017

stevengj commented Feb 8, 2017

stevengj commented Feb 10, 2017 • edited Loading

stevengj commented Feb 11, 2017

tkelman commented Feb 14, 2017 • edited Loading

ajkeller34 commented Feb 7, 2017 •

edited

Loading

GlenHertz Feb 8, 2017 •

edited

Loading

GlenHertz Feb 8, 2017 •

edited

Loading

stevengj commented Feb 10, 2017 •

edited

Loading

tkelman commented Feb 14, 2017 •

edited

Loading