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

Change behavior of `one` to match Unitful #42

Closed MilesCranmer closed 1 year ago

MilesCranmer commented 1 year ago

cc @mikeingold @stevengj

This is a draft PR to modify one(q) to return one(ustrip(q)) rather than a dimensionless quantity, to match the behavior of Unitful.jl. This also makes AbstractQuantity <: Number.

Fixes #40

Also @gaurav-arya what do you think? Apparently we need this for compatibility with packages that use Unitful.

TODO:

gaurav-arya commented 1 year ago

Hm, I do also appreciate the property one(::T) -> T. This would also help with type stability when writing e.g. a multiplicative accumulator (similarly to zero(::T) -> T often being relied upon for type stable sums). Note that prod is no longer type stable with this PR, due to the empty case using one for the default init value:

julia> q = 1u"kg/s"
1.0 kg s⁻¹

julia> arr_empty = typeof(q)[]
Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}[]

julia> arr = [q]
1-element Vector{Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}}:
 1.0 kg s⁻¹

julia> prod(arr_empty) |> typeof
Float64

julia> prod(arr) |> typeof
Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}

julia> @code_warntype prod(arr)
MethodInstance for prod(::Vector{Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}})
  from prod(a::AbstractArray; dims, kw...) @ Base reducedim.jl:996
Arguments
  #self#::Core.Const(prod)
  a::Vector{Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}}
Body::Union{Float64, Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}}
1 ─      nothing
│   %2 = Base.:(:)::Core.Const(Colon())
│   %3 = Core.NamedTuple()::Core.Const(NamedTuple())
│   %4 = Base.pairs(%3)::Core.Const(Base.Pairs{Symbol, Union{}, Tuple{}, @NamedTuple{}}())
│   %5 = Base.:(var"#prod#831")(%2, %4, #self#, a)::Union{Float64, Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}}
└──      return %5
MilesCranmer commented 1 year ago

Great point. Do you want to repost this on #40?

MilesCranmer commented 1 year ago

Will cherry pick the <: Number but otherwise this can be closed as QuadGK is going to switch to using BaseType.jl

MilesCranmer commented 1 year ago

Closing as we have BaseType.base_numeric_type now