Additional QuantityArray constructions
#178
Conversation
icweaver
changed the title
QuantityArray constructions
Benchmark Results (Julia v1.10)Time benchmarks
Memory benchmarks
|
Benchmark Results (Julia v1)Time benchmarks
Memory benchmarks
|
|
Hi @MilesCranmer, this PR should be ready for your review now. Two follow-up questions I had were:
|
|
Sweet, looks like this is working nicely with the new Makie PR: julia> using CairoMakie, DynamicQuantities
julia> x = [6, 7, 8]us"cm"
3-element QuantityArray(::Vector{Float64}, ::Quantity{Float64, SymbolicDimensions{FixedRational{Int32, 25200}}}):
6.0 cm
7.0 cm
8.0 cm
julia> y = (4:6)u"kg"
3-element QuantityArray(::StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}, ::Quantity{Float64, Dimensions{FixedRational{Int32, 25200}}}):
4.0 kg
5.0 kg
6.0 kg
julia> scatter(x, y)
|
Actually, think I found a good balance, just pushed. Does this look alright to you? |
MilesCranmer
left a comment
MilesCranmer
left a comment
There was a problem hiding this comment.
Looks good! Thanks for this and sorry for the delay. I only had a couple requests
|
Also re: your question, I think creating from generators is probably too much for now. |
Co-authored-by: Miles Cranmer <[email protected]>
Co-authored-by: Miles Cranmer <[email protected]>
|
Thanks Miles! Sorry for the mix-up with the README and generated index file; I think things have been straightened out now. In the process, I think I noticed that the HTML table of contents does not work as intended on my end at least, I think due to the case-sensitivity of the section headers. I'd be happy to open up a separate PR for this and maybe exploring the preprocessing step you mentioned for README code blocks if you think that would be useful |
|
Sure! Sounds great, thanks |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #178 +/- ##
==========================================
- Coverage 99.21% 95.62% -3.60%
==========================================
Files 21 20 -1
Lines 1273 1211 -62
==========================================
- Hits 1263 1158 -105
- Misses 10 53 +43 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
icweaver
changed the title
QuantityArray constructionsQuantityArray constructions [OLD]
icweaver
changed the title
QuantityArray constructions [OLD]QuantityArray constructions
|
Hi Miles, sorry for the delay on this. I think test coverage should be up now and things should be ready for another review |
| By default, DynamicQuantities will create a `QuantityArray` from an `AbstractArray`, similarly to how a `Quantity` is created from a scalar in the [Usage](@ref) examples: | ||
|
|
||
| ```julia | ||
| julia> x = [0.3, 0.4, 0.5]u"km/s" | ||
| 3-element QuantityArray(::Vector{Float64}, ::Quantity{Float64, Dimensions{FixedRational{Int32, 25200}}}): | ||
| 300.0 m s⁻¹ | ||
| 400.0 m s⁻¹ | ||
| 500.0 m s⁻¹ | ||
|
|
||
| julia> y = (42:45) * u"kg" | ||
| 4-element QuantityArray(::StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}, ::Quantity{Float64, Dimensions{FixedRational{Int32, 25200}}}): | ||
| 42.0 kg | ||
| 43.0 kg | ||
| 44.0 kg | ||
| 45.0 kg | ||
| ``` | ||
|
|
||
| This can be overridden to produce a vector of `Quantity`s by explicitly broadcasting the unit: | ||
|
|
||
| ```julia | ||
| julia> z = [0.3, 0.4, 0.5] .* u"km/s" | ||
| 3-element Vector{Quantity{Float64, Dimensions{FixedRational{Int32, 25200}}}}: | ||
| 300.0 m s⁻¹ | ||
| 400.0 m s⁻¹ | ||
| 500.0 m s⁻¹ | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Likely don't need 3 separate examples. I think just the first example is good. The rest can be deleted; users can figure it out by playing around (it's fairly intuitive I think
There was a problem hiding this comment.
Ok, dropped the last two examples 3c97dab
|
Rest is looking ok. One tricky thing is that I think this might be a breaking change (??). I'm not sure what to do with that in mind. |
test/unittests.jl
Outdated
| # Test multiplying ranges with units | ||
| x = (1:0.25:4)u"inch" | ||
| @test x isa StepRangeLen | ||
| @test x isa T_QA_StepRangeLen |
There was a problem hiding this comment.
| @test x isa T_QA_StepRangeLen | |
| @test x isa QuantityArray | |
| @test DQ.array_type(x) <: StepRangeLen | |
| @test DQ.dim_type(x) <: Dimensions |
bit more readable like this.
Same comment for other T_QA_AbstractArray types - we can swap them for using the interface to inspect the type parameters
There was a problem hiding this comment.
Ok, will swap these out. Sorry, by using the interface to inspect type parameters, is there something else that you would like to see instead?
For sure, just opened #188 |
|
Thanks for sticking with this and putting up with my many suggestions!! I made a couple minor changes. However, I did notice one issue before merging: julia> x = randn(3) .* u"km/s"
3-element Vector{Quantity{Float64, Dimensions{FixedRational{Int32, 25200}}}}:
1124.2145905299506 m s⁻¹
-451.8046249866915 m s⁻¹
1332.2748352376054 m s⁻¹
julia> x * u"km"
3-element QuantityArray(::Vector{Quantity{Float64, Dimensions{FixedRational{Int32, 25200}}}}, ::Quantity{Quantity{Float64, Dimensions{FixedRational{Int32, 25200}}}, Dimensions{FixedRational{Int32, 25200}}}):
(1.1242145905299506e6 m s⁻¹) m
(-451804.6249866915 m s⁻¹) m
(1.3322748352376055e6 m s⁻¹) mIn other words it seems like if you multiply a vector of quantities with a quantity, it becomes a quantity array containing internally a vector of quantities. Is this intentional or should this be handled somehow? Seems kinda tricky because certain things like Measurement{T} or Complex{T} might internally wrap a quantity... In which case I don't know how this could generically handle such things |
|
Fixed |
|
Hm... the load time is increased by over 10%. Not sure why. |
|
Ok the load time seems related to the many methods defined for different array types in the LinearAlgebra extension. It's still the case that LinearAlgebra is loaded automatically in Julia, so the extension doesn't really function as a dynamically loaded set of methods yet. Basically it just means this is something to fix later. But it's a high priority fix as the load time is painfully high for DQ (although it's still not near as bad as Unitful) |
|
Thanks again! |
|
Ah, interesting. Thanks for walking me through this PR, Miles; it taught me a lot! |
2 participants
Closes #166
To-do
QuantityArray#181)