SymbolicML / DynamicQuantities.jl

Efficient and type-stable physical quantities in Julia
https://symbolicml.org/DynamicQuantities.jl/dev/
Apache License 2.0
132 stars 17 forks source link

Compatibility with `zero(::Type{T})` #76

Open MilesCranmer opened 11 months ago

MilesCranmer commented 11 months ago

It seems like a reason DynamicQuantities.jl is incompatible with many libraries right now is due to their usage of zero(::Type{T}), which works for Unitful but not for DynamicQuantities.jl.

Therefore I wonder if there is a way of defining something like:

zero(::Type{Q}) where {Q<:AbstractQuantity} = AutoQuantityZero{Q}()

which could avoid checking for any dimension errors in operations, and act as a neutral zero.

The problem with this is type instability, but perhaps that's better than simply throwing an error...? Then as packages adapt they would get type stability again. (And of course any package already using zero(::T) would maintain type stability).

Thoughts @gaurav-arya?

We could even think of adding a special boolean field to Quantity to indicate this... thus making everything type stable. But it's a bit worrisome...

gaurav-arya commented 11 months ago

It doesn't necessarily have to be type unstable. For example, you could have a boolean valid bit in a Dimensions object, with valid = 0 in this case. I did something like this in StochasticAD.jl: https://github.com/gaurav-arya/StochasticAD.jl/blob/e13ee20616ec8cc053dab181ef9bccd160b0d12a/src/backends/pruned_aggressive.jl#L50-L53

In theory, the type approach + union splitting could also work. This could in theory be superior to the valid bit approach in that the branch will only be created in the case where one of the units could feasibly be invalid, as opposed to all the time for every addition of quantities. So this seems promising -- I just don't personally have a good mental model for when union splitting runs, its performance, etc.

Am I understanding correctly that we'd want to make the units of this zero a "wildcard", i.e willing to assume the units of any other type? It would make code such as zero(typeof(1u"m")) + 1u"s" work, but perhaps that's OK / a worthwhile tradeoff. [Edit: re-reading, yes I am pretty sure this is what you mean:)]

MilesCranmer commented 8 months ago

Pasted from #77

I think one alternative is to allow a user to temporarily disable dimensional analysis in a particular scope, so that the first call of a Base.:+(::Quantity, ::Number)would simply take the first dimensions it sees. Maybe like

using DynamicQuantities

@dimension_globbing begin
    u"km/s" + one(typeof(u"km/s"))
end

which would result as 1001 m/s. The one(u"km/s") == 1.0 but since you turned on dimension globbing, the 1.0 would automatically take the units m/s from the u"km/s" part.

mikeingold commented 8 months ago

Just came over to mention this occurs when trying to compose DynamicQuantities.jl with the geometric representations from Meshes.jl.

using DynamicQuantities
using Meshes

Meshes.Point(1.0m, 2.0m)

Example error stack trace:

ERROR: Cannot create an additive identity for a `UnionAbstractQuantity` type, as the dimensions are unknown. Please use `zero(::UnionAbstractQuantity)` instead.
Stacktrace:
 [1] error(s::String)
   @ Base .\error.jl:35
 [2] zero(::Type{DynamicQuantities.Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}})
   @ DynamicQuantities C:\Users\x\.julia\packages\DynamicQuantities\HYcKp\src\utils.jl:280
 [3] float(::Type{DynamicQuantities.Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}})
   @ Base .\float.jl:311
 [4] Vec(coords::Tuple{DynamicQuantities.Quantity{…}, DynamicQuantities.Quantity{…}})
   @ Meshes C:\Users\x\.julia\packages\Meshes\Lb9po\src\vectors.jl:46
 [5] Vec(::DynamicQuantities.Quantity{Float64, Dimensions{…}}, ::Vararg{DynamicQuantities.Quantity{…}})
   @ Meshes C:\Users\x\.julia\packages\Meshes\Lb9po\src\vectors.jl:58
 [6] Point(::DynamicQuantities.Quantity{Float64, Dimensions{…}}, ::Vararg{DynamicQuantities.Quantity{…}})
   @ Meshes C:\Users\x\.julia\packages\Meshes\Lb9po\src\primitives\point.jl:48
 [7] top-level scope
   @ REPL[15]:1
Some type information was truncated. Use `show(err)` to see complete types.
MilesCranmer commented 8 months ago

Thanks, I posted a comment there as well.

I'd be open to the idea https://github.com/SymbolicML/DynamicQuantities.jl/issues/76#issuecomment-1889384323 if someone wants to try it. It's not really a robust approach to this, but it is a temporary workaround in case there are issues like this in a downstream package.

mikeingold commented 8 months ago

Thanks again for the quick fix to float(T), @MilesCranmer !

I just updated my LineIntegrals.jl package to enable tests with DynamicQuantities.jl and have run into a new hurdle. I don't time to dig in more at the moment, but it looks like Julia's isapprox also runs into issues with zero(T) and something about Quantity<:Real, e.g.:

Scalar-Valued Functions: Error During Test at C:\Users\mikei\.julia\packages\LineIntegrals\Y8iYI\test\runtests.jl:159
  Test threw exception
  Expression: isapprox(integral(f, unit_circle), (2π) * Ω; atol = 0.15Ω)
  TypeError: in keyword argument atol, expected Real, got a value of type DynamicQuantities.Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}
  Stacktrace:
   [1] isapprox(l::DynamicQuantities.Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}, r::DynamicQuantities.Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}; kws::@Kwargs{atol::DynamicQuantities.Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}})
     @ DynamicQuantities C:\Users\mikei\.julia\packages\DynamicQuantities\jkfTz\src\utils.jl:227
   [2] eval_test(evaluated::Expr, quoted::Expr, source::LineNumberNode, negate::Bool)
     @ Test C:\Users\mikei\.julia\juliaup\julia-1.10.0+0.x64.w64.mingw32\share\julia\stdlib\v1.10\Test\src\Test.jl:367
   [3] macro expansion
     @ C:\Users\mikei\.julia\juliaup\julia-1.10.0+0.x64.w64.mingw32\share\julia\stdlib\v1.10\Test\src\Test.jl:669 [inlined]
   [4] macro expansion
     @ C:\Users\mikei\.julia\packages\LineIntegrals\Y8iYI\test\runtests.jl:159 [inlined]
   [5] macro expansion
     @ C:\Users\mikei\.julia\juliaup\julia-1.10.0+0.x64.w64.mingw32\share\julia\stdlib\v1.10\Test\src\Test.jl:1577 [inlined]
   [6] macro expansion
     @ C:\Users\mikei\.julia\packages\LineIntegrals\Y8iYI\test\runtests.jl:154 [inlined]
   [7] macro expansion
     @ C:\Users\mikei\.julia\juliaup\julia-1.10.0+0.x64.w64.mingw32\share\julia\stdlib\v1.10\Test\src\Test.jl:1577 [inlined]
   [8] top-level scope
     @ C:\Users\mikei\.julia\packages\LineIntegrals\Y8iYI\test\runtests.jl:128

Vector-Valued Functions: Error During Test at C:\Users\mikei\.julia\packages\LineIntegrals\Y8iYI\test\runtests.jl:164
  Test threw exception
  Expression: integral(f, seg_ne) ≈ [sqrt(2), sqrt(2), sqrt(2)] .* Ω
  Cannot create an additive identity for a `UnionAbstractQuantity` type, as the dimensions are unknown. Please use `zero(::UnionAbstractQuantity)` instead.
  Stacktrace:
    [1] error(s::String)
      @ Base .\error.jl:35
    [2] zero(::Type{DynamicQuantities.Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}})
      @ DynamicQuantities C:\Users\mikei\.julia\packages\DynamicQuantities\jkfTz\src\utils.jl:280
    [3] real(T::Type)
      @ Base .\complex.jl:120
    [4] rtoldefault(x::Type{DynamicQuantities.Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}}, y::Type{DynamicQuantities.Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}}, atol::Int64)
      @ Base .\floatfuncs.jl:347
    [5] isapprox(x::Vector{DynamicQuantities.Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}}, y::Vector{DynamicQuantities.Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}})
      @ LinearAlgebra C:\Users\mikei\.julia\juliaup\julia-1.10.0+0.x64.w64.mingw32\share\julia\stdlib\v1.10\LinearAlgebra\src\generic.jl:1785
    [6] eval_test(evaluated::Expr, quoted::Expr, source::LineNumberNode, negate::Bool)
      @ Test C:\Users\mikei\.julia\juliaup\julia-1.10.0+0.x64.w64.mingw32\share\julia\stdlib\v1.10\Test\src\Test.jl:355
    [7] macro expansion
      @ C:\Users\mikei\.julia\juliaup\julia-1.10.0+0.x64.w64.mingw32\share\julia\stdlib\v1.10\Test\src\Test.jl:669 [inlined]
    [8] macro expansion
      @ C:\Users\mikei\.julia\packages\LineIntegrals\Y8iYI\test\runtests.jl:164 [inlined]
    [9] macro expansion
      @ C:\Users\mikei\.julia\juliaup\julia-1.10.0+0.x64.w64.mingw32\share\julia\stdlib\v1.10\Test\src\Test.jl:1577 [inlined]        
   [10] macro expansion
      @ C:\Users\mikei\.julia\packages\LineIntegrals\Y8iYI\test\runtests.jl:163 [inlined]
   [11] macro expansion
      @ C:\Users\mikei\.julia\juliaup\julia-1.10.0+0.x64.w64.mingw32\share\julia\stdlib\v1.10\Test\src\Test.jl:1577 [inlined]        
   [12] top-level scope
      @ C:\Users\mikei\.julia\packages\LineIntegrals\Y8iYI\test\runtests.jl:128
mikeingold commented 8 months ago

Looks like both of those errors stem from isapprox implementation of rtol and atol settings here, I.e.

atol::Real=0, rtol::Real=rtoldefault(x,y,atol)

where rtoldefault here calls real(T).

Edit: I went back and forked this into its own Issue with a MWE.