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

Change behavior of one to match Unitful #42

Closed
wants to merge 7 commits into from

Conversation

MilesCranmer
Copy link
Member

@MilesCranmer MilesCranmer commented Sep 19, 2023

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:

  • Turn on other unittests again (will require updating the assumed behavior of one)

@MilesCranmer MilesCranmer changed the title Change behavior of one for compatibility with QuadGK.jl Change behavior of one to match Unitful Sep 19, 2023
@gaurav-arya
Copy link
Collaborator

gaurav-arya commented Sep 19, 2023

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}}}}
1nothing%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
Copy link
Member Author

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

@MilesCranmer
Copy link
Member Author

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

@MilesCranmer
Copy link
Member Author

Closing as we have BaseType.base_numeric_type now

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.

Quantity currently incompatible with QuadGK
2 participants