-
Notifications
You must be signed in to change notification settings - Fork 20
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
Conversation
Benchmark Results
Benchmark PlotsA plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR. |
The |
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... |
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.
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()) |
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.
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, "[*]") |
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.
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)
Base.getproperty(::WildcardDimensions, ::Symbol) = zero(Bool) | ||
Base.iszero(::WildcardDimensions) = true |
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.
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)
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.") |
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 using DynamicQuantities
@dimension_globbing begin
u"km/s" + one(u"km/s")
end which would result as |
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:Then, it makes
zero
for any quantity type simply setWildcardDimensions
as the dimensions:which makes all dimensionality checks pass automatically:
That's pretty much it. Then you can do things like:
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 haveoneunit(::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 thinkoneunit
would be safe to use this way.