-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
add Test.GenericDimensionful number type #39852
base: master
Are you sure you want to change the base?
Conversation
The remaining CI failures seem unrelated. On
on
and on
|
Do we know that there are packages that use |
According to juliahub.com, the only package that uses either one is Statistics.jl (which uses Furlong). |
Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>
Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>
Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>
Should we get a second opinion on the |
Should we have GenericDimensionful{2}(1) == GenericDimensionful{2.0}(1) == GenericDimensionful{2//1}(1) or should we expect that people only use the canonical form ( Edit: I see that the above equality should now hold via promotion. |
@test_throws DimensionMismatch x == GD{2}(4) | ||
@test_throws ErrorException x == 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, these shouldn’t error, they should return false
(I don’t think ==
ever errors). Unitful and Dates also return false
in this case.
Adding equivalent tests for isequal
would also be good.
Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>
I think a lot of things would be easier if we only allowed the canonical type for any |
Sorry, I closed it on accident. |
Since we have a custom |
If we want to allow non-canonical exponents, we need to fix this method ambiguity: julia> promote(GenericDimensionful{0}(2), GenericDimensionful{0.0}(2))
ERROR: MethodError: promote_rule(::Type{GenericDimensionful{0, Int64}}, ::Type{GenericDimensionful{0.0, Int64}}) is ambiguous. Candidates:
promote_rule(::Type{GenericDimensionful{p, T}}, ::Type{GenericDimensionful{q, S}}) where {p, q, T, S} in Test.GenericDimensionfuls at /home/sebastian/Development/julia/usr/share/julia/stdlib/v1.7/Test/src/GenericDimensionfuls.jl:43
promote_rule(::Type{GenericDimensionful{0, T}}, ::Type{S}) where {T, S<:Number} in Test.GenericDimensionfuls at /home/sebastian/Development/julia/usr/share/julia/stdlib/v1.7/Test/src/GenericDimensionfuls.jl:46
Possible fix, define
promote_rule(::Type{GenericDimensionful{0, T}}, ::Type{GenericDimensionful{q, S}}) where {T, q, S} |
end | ||
GenericDimensionful(x::T) where {T<:Number} = GenericDimensionful{1,T}(x) | ||
GenericDimensionful(x::GenericDimensionful) = x | ||
(::Type{T})(x::GenericDimensionful) where {T<:Number} = T(x.val)::T |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(::Type{T})(x::GenericDimensionful) where {T<:Number} = T(x.val)::T | |
(::Type{T})(x::GenericDimensionful{0}) where {T<:Number} = T(x.val)::T |
So that we don’t convert dimensionful numbers to dimensionless ones.
Are we still interested in these fixes, or what stalled it? Furlong has picked up a few bugfixes, so those will need to be moved into the new type here |
Closes #30692.
As commented in #27899 (comment), we had actually defined
threefour different unitful types for tests (Furlong
,PhysQuantity
,MeterUnits
, andMockUnitful
). This unifies them all into a single typeGenericDimensionful
that is exported fromTest
.(This is analogous to
GenericString
,GenericDict
, etcetera — the goal is to make it easy for package developers to test that their code is generic to new number types without having to think about which additional dependency to pull in.)Along the way, I discovered a few bugs in the support of dimensionful types by Base and LinearAlgebra functions and I fixed them. (The basic issue is that you should not assume you can
convert
between types with different dimensions, and should only rely on constructors.)const Furlong = Test.GenericDimensionful
stub definition because it is needed by the Statistics.jl tests. Once Statistics.jl is updated we can change this to a deprecation. (Or delete it — according to juliahub, no other packages use it?)