Segfault on DynamicQuantities range #56610
Comments
It's an out-of-bound issue, there's probably an unsafe |
|
I'm guessing that this is a Floating point range issue (i.e. we are |
|
DynamicQuantities is somehow creating a StepRange instead of StepRangeLen and StepRange is not supposed to use floats. |
|
looks like that is the default behavior. In range.jl there's while the code that creates StepRangeLen is specifically for |
|
Should DynamicQuantities maybe borrow some code from Unitful.jl? Edit: Probably not. Seems like it's all pirated #56610 (comment) |
julia> x = 1:0.25:4;
julia> xu = 1u"inch":0.25u"inch":4u"inch";
julia> xval = (1u"inch".value):(0.25u"inch".value):(4u"inch".value)
0.025400000000000002:0.006350000000000001:0.10160000000000001
julia> length(x), length(xu), length(xval)
(13, 12, 13)
julia> @less Vector{eltype(xu)}(xu)This iterates the range and indexes function Array{T,1}(r::AbstractRange{T}) where {T}
a = Vector{T}(undef, length(r))
i = 1
for x in r
@inbounds a[i] = x
i += 1
end
return a
endAnd the iteration for StepRange assumes that iterate(r::OrdinalRange) = isempty(r) ? nothing : (first(r), first(r))
function iterate(r::OrdinalRange{T}, i) where {T}
@inline
i == last(r) && return nothing
next = convert(T, i + step(r))
(next, next)
end |
|
It looks like we can solve this by defining but it is definitely a bug in Base that this could happen in the first place without DynamicQuantities doing anything wrong. |
|
Yes, seems like Base should not create a StepRange unless it's certain that the types obey its rules. Nicest path for DynamicQuantities to encourage is probably something like this, so that floating point magic happens on the numbers as written, before conversion, not after: julia> (1:0.25:4)u"inch"
ERROR: Cannot create an additive identity from `Type{<:Quantity}`, as the dimensions are unknown. Please use `zero(::Quantity)` instead. |
|
Seriously impressed at everyone's diligence as applied to this imperfectly-reported bug. Bravo! I was suspicious that this error should concern Julia core/base because of the accessibility of the segfault. I appreciate everyone's validation on this point. Separately, should I open a report over at DynamicQuantities.jl with any suggestion(s)? What I have seen suggested so far seems like documentation-type fixes. If instead the goal is to have the above code work as-written, I wonder what can be done? One solution would be to override the colon and double-colon operators to "lower" coloning to floating point types underlying a Quantity (as partially suggested by @oscardssmith), then to have a slim wrapper type of some kind until the sequence must be realized. Do I understand? I have not looked at how Unitful.jl does it (as suggested by @christiangnrd) but can look and propose to DynamicQuantities.jl if this was another intended solution to make the original code work:
Note that this originally arose in a situation where these values were variables with different units, so I felt motivation to report this specific case and feel motivated to help it work. |
|
looks like Unitful solves this by just pirating all of the Base range code https://github.com/PainterQubits/Unitful.jl/blob/b53bc81daea02c55e625c337962087e0f3af63d9/src/range.jl.. That's definitely not a great solution. |
|
I think one fix to this issue would be if the stdlib had: - TwicePrecision{T}(x::T) where {T} = TwicePrecision{T}(x, zero(T))
+ TwicePrecision{T}(x::T) where {T} = TwicePrecision{T}(x, zero(x))Going to put this in DQ for AbstractQuantity at least |
|
If I define: function Base.:(:)(start::T, step::T, stop::T) where {T<:$type}
range(start, stop, length=length(start.value:step.value:stop.value))
endthen it will still break for mixed units/integers (which are valid – julia> collect(1:0.25u"1":4u"1")
ERROR: MethodError: no method matching (::Colon)(::Int64, ::Quantity{Float64, Dimensions{FixedRational{…}}}, ::Quantity{Float64, Dimensions{FixedRational{…}}})
The function `Colon()` exists, but no method is defined for this combination of argument types.
Closest candidates are:
(::Colon)(::T, ::T, ::T) where T<:AbstractQuantity
@ DynamicQuantities ~/PermaDocuments/SymbolicRegressionMonorepo/DynamicQuantities.jl/src/math.jl:63
(::Colon)(::T, ::Any, ::T) where T<:Real
@ Base range.jl:50
(::Colon)(::A, ::Any, ::C) where {A<:Real, C<:Real}
@ Base range.jl:10
...I guess I would need to loop over types for all |
|
ah the dispatch hiercarchy here is unfortunate. ideally, there would be a fallback method here that would promote... |
I may have missed it, but which method is exactly type piracy? I see a bunch of non-public functions being extended, and that's not ideal (probably an indication it couldn't be done in a better way), but as far as I can see at a quick glance all signatures include types defined by the package, didn't looks to me full blown type piracy. |
|
From @mcabbott message I understand that this is a problematic function. function Array{T,1}(r::AbstractRange{T}) where {T}
a = Vector{T}(undef, length(r))
i = 1
for x in r
@inbounds a[i] = x
i += 1
end
return a
endThis may sound naive, but why no one considers simply removing the problematic |
|
Julia also typically claims that there is nothing really special about the Base types but having it only use "fast" versions of its algorithms on Base types goes a but against that. |
I do think an object lying about its Certainly the fallback path of Possibly the iteration should be made more robust by checking |
|
Is one of the underlying questions here how to exactly predict the length of a sequence Heaven forbid there ever be a geometric series, or a sufficiently small step that someone tries to repeat history, or an additive type that accumulates undue floating point error under the hood; unfortunately, heaven will not save us from these eventualities, only documentation might. |
|
Things like |
oscardssmith
added
bug
|
Am implementing @oscardssmith's suggested workaround in DQ with the following code: # Needed for multiplying range with quantities:
Base.TwicePrecision{T}(x::T) where {T<:AbstractQuantity} = Base.TwicePrecision{typeof(x)}(x, zero(x))
# Needed for robust ranges from quantities:
for T1 in (AbstractQuantity{<:Real}, Real),
T2 in (AbstractQuantity{<:Real}, Real),
T3 in (AbstractQuantity{<:Real}, Real)
T1 === T2 === T3 === Real && continue
@eval function Base.:(:)(start::$T1, step::$T2, stop::$T3)
return range(start, stop, length=length(ustrip(start):ustrip(step):ustrip(stop)))
end
end
which will force a StepRangeLen. One quirk is that cases like julia> range(1, 5, length=1)
ERROR: ArgumentError: range(1.0, stop=5.0, length=1): endpoints differ
Stacktrace:
[1] _linspace1(::Type{Float64}, start::Float64, stop::Float64, len::Int64)
@ Base ./twiceprecision.jl:737
[2] _linspace(::Type{Float64}, start_n::Int64, stop_n::Int64, len::Int64, den::Int64)
@ Base ./twiceprecision.jl:717
[3] _linspace
@ ./twiceprecision.jl:714 [inlined]
[4] range_start_stop_length
@ ./range.jl:597 [inlined]
[5] _range
@ ./range.jl:167 [inlined]
[6] #range#84
@ ./range.jl:150 [inlined]
[7] top-level scope
@ REPL[25]:1 |
@MilesCranmer, quick sanity check for symbolic units: using DynamicQuantities
(a, b, c) = 1u"ft", 1u"inch", 1u"m" # (0.3048 m, 0.025400000000000002 m, 1.0 m)
ustrip(a):ustrip(b):ustrip(c) # 0.3048:0.025400000000000002:0.9906000000000001
(as, bs, cs) = 1us"ft", 1us"inch", 1us"m" # (1.0 ft, 1.0 inch, 1.0 m)
ustrip(as):ustrip(bs):ustrip(cs) # 1.0:1.0:1.0 |
|
Nice catch! Edit: seems like I am just missing a check for compatible units. @eval function Base.:(:)(start::$T1, step::$T2, stop::$T3)
dimension(start) == dimension(step) || throw(DimensionError(start, step))
dimension(start) == dimension(stop) || throw(DimensionError(start, stop))
return range(start, stop, length=length(ustrip(start):ustrip(step):ustrip(stop)))
end |
Unexpected segfault:
The text was updated successfully, but these errors were encountered: