-
Notifications
You must be signed in to change notification settings - Fork 19
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
Support dispatches on Number
via union type
#49
Conversation
Benchmark Results
Benchmark PlotsA plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR. |
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.
This is an extremely cool PR: I don't know of any other number type packages that have made the effort to work around the MyStruct{T} <: T
conundrum. (the closest might be symbolics having both generic variables and a special Num <: Real
?).
I have admittedly not had time yet to review line by line, but some general comments:
Naming: TL;DR everything seems fine:) I wondered whether AbstractUnionQuantity
should really be AbstractQuantity
. However, I think it's important for it to be clear that this is a union type (and can't e.g. be subtyped), so I think you've already made the right choice. Relatedly, should AbstractQuantity <: Number
really be AbstractNumberQuantity <: Number
. I'm fine with your choice here too: special casing Number
as the shortest name / default type seems reasonable, as that's what Unitful
worked with. Presumably future additions would look like AbstractRealQuantity <: Real
etc.
Code readability: I don't think code readability is hurt that much in this PR (the net increase in code lines in impressively small). Further, I hope in a future PR to substantially reduce the code size of e.g.math.jl
by classifying the overloads into their maybe 3 or 4 different kinds and then having a simple loop over functions for each (or a convenience macro to define the overload). #64 is also somewhat related. That would hopefully increase readability further, as what we're seeing now is the multiplicative explosion of the looping logic together with the original repeated-overload complexity.
Unit parsing: Right now, AFAIU unit strings are always parsed as an instance of Quantity
. This will be an interesting thing to revisit if were to want e.g. an AbstractRealQuantity
in the future, as we may want 1.0 * u"m" <: Real
for example. But I don't think there's any issue right now.
Awesome, thanks so much @gaurav-arya!!
Exactly my thoughts as well. i.e., keeping it
Great idea! We could use TypedDelegation.jl for this too if needed.
Sounds good, we can bring this part up again later! |
Not sure what’s going on with the automated benchmarks, but if I run the benchmarks locally it seems to have no effect on the performance. Although I’m using 1.10 locally so perhaps there’s some difference on 1.9... |
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.
Some small comments:)
The performance regression on 1.9 appears to be real: julia> using DynamicQuantities
julia> q = 1u"m"
1.0 m
julia> @btime inv($q)
254.961 ns (8 allocations: 768 bytes) |
1abbc8d
to
3bb9452
Compare
I fixed the performance regression. Looks like it was just something that the Julia compiler was able to inline on Julia 1.10 but not the earlier versions. I just moved it to a generated function now. |
Thanks for the review! Here are the changes since your last pass over: 3bb9452...union-type Regarding test coverage, it is good to worry about but I'm not that worried because Also, just to be safe, I'm looping over both quantity types in a lot of the unittests now, particularly the basic tests and the QuantityArray tests. |
Btw, should it be Also, maybe |
Okay I changed it to Any last comments or do you think it's good to go? |
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.
Sorry for the slow review -- nearly all looks good to me! I just have a note about the fieldnames check.
Thanks for the reviews!! |
This fixes #44 and refactors much of the internals to support two abstract quantities:
AbstractQuantity{T,D} <: Number
. Same name, but now onlyNumber
AbstractGenericQuantity{T,D}
. New name, supportsAny
It does this by creating
UnionAbstractQuantity
which is a union of these two types:There is also now
GenericQuantity
as a built-inAbstractGenericQuantity
, similar to howQuantity
is the built-inAbstractQuantity
.In principle one could also add
RealQuantity <: AbstractRealQuantity
and so on. I chose to not add those yet until someone needs it.@gaurav-arya what do you think?
The downside of this PR is that it hurts readability due to the use of
@eval
(required to avoid method ambiguities, and if we don't want to manually write a hundred extra methods...)