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

[WIP] Create wildcard dimensions for zero(::Type) #77

Closed
wants to merge 1 commit into from

Conversation

MilesCranmer
Copy link
Member

@MilesCranmer MilesCranmer commented Nov 2, 2023

So in principle this will solve the compatibility issue with having zero(::Type{T}) sprinkled throughout various libraries. Fixes #76.

But I am worried this might introduce bugs that are silent rather than loud as they are now...

@gaurav-arya do you think you could take a look when you get a chance?


The PR solves this issue in a simple way. It creates a special AbstractDimensions type for wildcard dimensions:

struct WildcardDimensions{R} <: AbstractDimensions{R}
    WildcardDimensions() = new{Bool}()
end

Then, it makes zero for any quantity type simply set WildcardDimensions as the dimensions:

Base.zero(::Type{Q}) where {T,Q<:UnionAbstractQuantity{T}} = constructorof(Q)(zero(T), WildcardDimensions())

which makes all dimensionality checks pass automatically:

# Wildcard dimensions are always compatible:
Base.:(==)(::AbstractDimensions, ::WildcardDimensions) = true
Base.:(==)(::WildcardDimensions, ::AbstractDimensions) = true

# For propagation rules, we assume all relevant dimensions are zero
Base.getproperty(::WildcardDimensions, ::Symbol) = zero(Bool)
Base.iszero(::WildcardDimensions) = true

That's pretty much it. Then you can do things like:

julia> x = 1u"km/s"
1000.0 m s⁻¹

julia> x + zero(typeof(x))
1000.0 m s⁻¹

julia> x * zero(typeof(x))
0.0 m s⁻¹

whereas before this would simply fail.

I thought about adding this for oneunit as well, but there I'm a bit worried since you could potentially have oneunit(::Type) take the dimensions of the first quantity it comes into contact with, rather than the type of the input. Since all the units in this library are zero at zero (we don't have Celsius), zero seems safe to use with wildcards. But I don't think oneunit would be safe to use this way.

Copy link
Contributor

github-actions bot commented Nov 2, 2023

Benchmark Results

main 5e69fde... t[main]/t[5e69fde...]
Quantity/creation/Quantity(x) 3 ± 0.1 ns 3.1 ± 0.1 ns 0.968
Quantity/creation/Quantity(x, length=y) 3.4 ± 0 ns 3.4 ± 0 ns 1
Quantity/with_numbers/*real 3.4 ± 0 ns 3.4 ± 0 ns 1
Quantity/with_numbers/^int 11.4 ± 3.7 ns 11.1 ± 3.6 ns 1.03
Quantity/with_numbers/^int * real 12.5 ± 4.4 ns 12.4 ± 4.4 ns 1.01
Quantity/with_quantity/+y 7.7 ± 0.001 ns 7.7 ± 0.1 ns 1
Quantity/with_quantity//y 3.4 ± 0 ns 3.4 ± 0 ns 1
Quantity/with_self/dimension 1.7 ± 0 ns 1.7 ± 0 ns 1
Quantity/with_self/inv 3.4 ± 0 ns 3.4 ± 0 ns 1
Quantity/with_self/ustrip 1.7 ± 0 ns 1.7 ± 0 ns 1
QuantityArray/broadcasting/multi_array_of_quantities 0.223 ± 0.17 ms 0.222 ± 0.16 ms 1
QuantityArray/broadcasting/multi_normal_array 0.0812 ± 0.005 ms 0.081 ± 0.0051 ms 1
QuantityArray/broadcasting/multi_quantity_array 0.254 ± 0.0012 ms 0.254 ± 0.0012 ms 1
QuantityArray/broadcasting/x^2_array_of_quantities 0.0418 ± 0.0033 ms 0.0413 ± 0.0037 ms 1.01
QuantityArray/broadcasting/x^2_normal_array 8.1 ± 1.7 μs 8.5 ± 1.6 μs 0.953
QuantityArray/broadcasting/x^2_quantity_array 10 ± 1.2 μs 10.1 ± 1.1 μs 0.99
QuantityArray/broadcasting/x^4_array_of_quantities 0.136 ± 0.0066 ms 0.136 ± 0.0066 ms 1
QuantityArray/broadcasting/x^4_normal_array 0.0685 ± 0.0006 ms 0.0684 ± 0.0006 ms 1
QuantityArray/broadcasting/x^4_quantity_array 0.0863 ± 0.0032 ms 0.0863 ± 0.0019 ms 1
time_to_load 0.203 ± 0.0003 s 0.204 ± 0.00092 s 0.995

Benchmark Plots

A plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR.
Go to "Actions"->"Benchmark a pull request"->[the most recent run]->"Artifacts" (at the bottom).

@gaurav-arya
Copy link
Collaborator

The x * zero(typeof(x)) example doesn't quite seem right to me: why should the wildcard become dimensionless in that case? Would it be better for the wildcard to only support operations like + where all inputs are expected to have the same dimension?

@MilesCranmer
Copy link
Member Author

Good point. Yeah on second thought I’m not even sure of the utility here... Just adding zeros doesn’t have practical value, but every other use case will have some sort of silent bug now...

Copy link
Collaborator

@gaurav-arya gaurav-arya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I guess it depends on what downstream uses of zero(T) look like in real life. My intuition is actually that most cases would be covered by handling only ops that expect the same dimensions for all arguments, although I could be totally off.

Whether the more permissive behaviour is worth it in that case, I am not sure -- but I think it's worth investigating in practice a bit further. To me, a big question is whether the union splitting of something like Quantity{T, Union{WildcardDimensions, AbstractDimensions}} is going to be somewhat efficient. If it kills performance, then I don't think the more permissive behaviour is worth it -- OTOH, if union splitting + maybe even some friendly branch prediction can substantially reduce the cost, then adding the more permissive behaviour for + and friends looks somewhat appealing. (Still with a clear error message for things like *, see a suggestion below.)

Base.:(==)(::WildcardDimensions, ::AbstractDimensions) = true

# For any zero/oneunit specified on the type, we return a wildcard dimension instead:
Base.zero(::Type{Q}) where {T,Q<:UnionAbstractQuantity{T}} = constructorof(Q)(zero(T), WildcardDimensions())
Copy link
Collaborator

@gaurav-arya gaurav-arya Nov 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the right type for the returned value should be along the lines of Quantity{T, Union{WildcardDimensions, Dimensions}}. So that code such as the below would work:

using DynamicQuantities

x = 1u"ms"
arr = [zero(typeof(x))]
arr[1] = x

(Right now, it gives a StackOverflow error, but I presume that's just because the PR is WIP.)

Base.promote_rule(::Type{D}, ::Type{<:WildcardDimensions}) where {D<:AbstractDimensions} = D

# Other utility rules:
Base.show(io::IO, ::WildcardDimensions) = print(io, "[*]")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If dimension(x) is printed with x having WildcardDimensions, this looks like an array (totally unimportant since this is WIP, but just writing before I forget)

Comment on lines +17 to +18
Base.getproperty(::WildcardDimensions, ::Symbol) = zero(Bool)
Base.iszero(::WildcardDimensions) = true
Copy link
Collaborator

@gaurav-arya gaurav-arya Nov 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These lines seem too dangerous as discussed. Maybe getproperty should just be replaced with an error: (this is not very carefully considered, just a thought)

Suggested change
Base.getproperty(::WildcardDimensions, ::Symbol) = zero(Bool)
Base.iszero(::WildcardDimensions) = true
Base.getproperty(::WildcardDimensions, ::Symbol) = error("Cannot get dimensions of quantity with WildcardDimensions. This is probably because the quantity was created via `zero(T)` for a type `T <: AbstractQuantity`. Instead, use `zero(x)` where `x` is an `AbstractQuantity` with the desired dimensions.")

@MilesCranmer
Copy link
Member Author

I've thought about this a bit more. I think it likely be a nightmare to have working due to associativity being broken.

However I think one alternative is to allow a user to temporarily disable dimension error rules 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(u"km/s")
end

which would result as 1001 m/s.

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.

Compatibility with zero(::Type{T})
2 participants