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? |
|
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:
|
|
Added a to-do for the method ambiguities picked up by Aqua Note to self, related reading: |
|
Hi @MilesCranmer, definitely not the most elegant, but I think the remaining method ambiguities have been addressed now and that this PR is ready for another review |
|
Sorry is the Dates extension new? I don’t think I noticed that before. Could you put the Dates.jl extension in a separate PR please? Sorry for not mentioning earlier, I didn’t notice |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #178 +/- ##
==========================================
- Coverage 99.21% 98.83% -0.39%
==========================================
Files 21 21
Lines 1273 1284 +11
==========================================
+ Hits 1263 1269 +6
- Misses 10 15 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
docs/Project.toml
Outdated
| Documenter = "1" | ||
|
|
||
| [sources] | ||
| DynamicQuantities = {path = ".."} |
There was a problem hiding this comment.
DynamicQuantities doesn’t need to be specified here. (Also ideally this would go in a separate PR if possible, since it’s unrelated to the QuantityArray stuff)
There was a problem hiding this comment.
Oh yep, that snuck in by mistake, my bad. Looking forward to it being included in Abhro's PR =]
|
Hm I think the test coverage CI should fail, I think the repo change means codecov isn’t aware of the current coverage |
Yep, I introduced it in 096ca12 while playing method ambiguity whac-a-mole with Aqua. sg, will split into its own PR Updated: Started #181 |
|
Hi @MilesCranmer, since the Dates.jl stuff covered some method ambiguities introduced by this PR, tests are understandably failing here now. Is this just an unavoidable consequence of relaxing the multiplication rules so that |
|
For such a large number of method ambiguities I think it could be because the new methods introduced are too generic. If we are going so far as to need package extensions just to fix ambiguities, my gut feeling is that we are only fixing the symptoms, but not the cause (which could very well be my fault) |
|
Oh sorry, I should have tried out tighter subtyping first instead of just blindly patching things with Aqua. Restricting to abstract arrays of I'll go ahead and close the superfluous Dates.jl draft PR The handful of patches here are still needed to cover the remaining method ambiguities, but I hope that's not a deal breaker # DynamicQuantities.jl/src/math.jl
Base.:*(A::StepRangeLen{<:Real, <:Base.TwicePrecision}, q::AbstractRealQuantity) = QuantityArray(A, q)
Base.:*(q::AbstractRealQuantity, A::StepRangeLen{<:Real, <:Base.TwicePrecision}) = A * q
Base.:/(A::BitArray, q::AbstractRealQuantity) = A * inv(q)
Base.:/(A::BitArray, q::AbstractQuantity) = A * inv(q)
Base.:/(A::StepRangeLen{<:Real, <:Base.TwicePrecision}, q::AbstractRealQuantity) = A * inv(q) |
src/arrays.jl
Outdated
| @@ -14,6 +14,14 @@ and so can be used in most places where a normal array would be used, including | |||
| # Constructors | |||
| The most convenient way to create a `QuantityArray` is by multiplying your array-like object by the desired dimension(s), e.g., | |||
| using TestItems: @testitem | ||
|
|
||
| DQ.is_ext_loaded(::Val{:LinearAlgebra}) = true | ||
| DQ.norm(u) = LA.norm(u) | ||
| LA.norm(q::UnionAbstractQuantity, p::Real=2) = new_quantity(typeof(q), LA.norm(ustrip(q), p), dimension(q)) | ||
|
|
||
| const ARRAY_TYPES_CONCRETE = ( |
There was a problem hiding this comment.
Looking at this now, I guess it should just be inlined, rather than having a constant for it.. to be consistent with the other for loop.
src/math.jl
Outdated
| @@ -49,9 +49,21 @@ for (type, base_type, _) in ABSTRACT_QUANTITY_TYPES | |||
| function Base.:/(l::AbstractDimensions, r::$type) | |||
| new_quantity(typeof(r), inv(ustrip(r)), l / dimension(r)) | |||
| end | |||
|
|
|||
| # Defining here instead of outside loop with UnionAbstractQuantity to avoid method ambiguities | |||
| Base.:*(A::AbstractArray{T}, q::$type) where {T<:Number} = QuantityArray(A, q) | |||
There was a problem hiding this comment.
This should be in the arrays file; this file is just scalar math
src/math.jl
Outdated
| end | ||
| end | ||
|
|
||
| # Deal with ambiguous QuantityArray operations: | ||
| Base.:*(A::StepRangeLen{<:Real, <:Base.TwicePrecision}, q::AbstractRealQuantity) = QuantityArray(A, q) |
There was a problem hiding this comment.
Similar comment - should be in the arrays file near the related operators
|
Sick, thanks Miles! Your comments should be in now. Happy to add more tests, but just wanted to make sure I was adding them to the right places first 👍🏾 |
src/arrays.jl
Outdated
| @@ -441,6 +459,13 @@ for op in (:(Base.:*), :(Base.:/), :(Base.:\)) | |||
| end | |||
| end | |||
|
|
|||
| # Cover method ambiguities from, e.g., op(::Array, ::Quantity)::QuantityArray` | |||
There was a problem hiding this comment.
Disambiguities should go in src/disambiguities
There was a problem hiding this comment.
To be more specific: the new “ Eagerly construct a QuantityArray from the following:” part can stay here. But specific disambiguities should go in the disambiguities file
There was a problem hiding this comment.
Roger! Oh nice, there's even an Aqua section 😄
src/arrays.jl
Outdated
| @@ -60,6 +68,16 @@ for (type, base_type, default_type) in ABSTRACT_QUANTITY_TYPES | |||
| if type in (AbstractQuantity, AbstractGenericQuantity) | |||
| @eval QuantityArray(v::AbstractArray{<:$base_type}, d::AbstractDimensions) = QuantityArray(v, d, $default_type) | |||
| end | |||
|
|
|||
| # Eagerly construct a `QuantityArray` from the following: | |||
| # ∘ array * unit | |||
There was a problem hiding this comment.
Comments like this aren’t needed
|
Okie doke, next round should be up in the latest commit 227ece9 |
Closes #166
To-do
QuantityArray#181)