-
Notifications
You must be signed in to change notification settings - Fork 25
Create QuantityArray <: AbstractArray
#33
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
Conversation
Benchmark Results
Benchmark PlotsA plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR. |
b68ca3e
to
cc945bc
Compare
Seems like a bunch of the array tests pass on 1.10 but don't on 1.9... Maybe something in the broadcasting interface changed. |
b2feddd
to
60bf0b5
Compare
Woo! 100% testing coverage again! I'll do another code review and then we can merge. |
Base.promote(x::Integer, y::F) where {F<:FixedRational} = (F(x), y) | ||
Base.promote(x::F, y::Integer) where {F<:FixedRational} = reverse(promote(y, x)) |
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.
My one comment is that these should probably be turned into promote_rule
at some point. But that's for another PR.
Also I realize this specific and unusual promotion requirement might make things a bit harder to move FixedRational
into Ratios.jl (which I still think is a good idea, cc @oscardssmith)... But we can just figure that out later.
Thanks again for your help @gaurav-arya!! |
This creates a type:
that stores a single set of physical dimensions for the entire array.
For example:
it implements a custom broadcasting interface so as to create a new
QuantityArray
with the correct output dimensions. This part is a bit involved as it needs to materialize the first element. Thanks to tictaccat on Discourse for help.Everything is type stable.
For safety purposes, the compiler is in charge of figuring out when the dimension calculation can be skipped for each element or not. It should be able to if it inlines things correctly, which happens for simple broadcasting. For more complex broadcasting it might not be able to inline things.
@ChrisRackauckas do you think you or someone else in @SciML could review this to see if it fits your requirements?