Skip to content
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

Add missing Base.similar(::QuantityArray, ...) definition #95

Merged
merged 10 commits into from
Dec 2, 2023

Conversation

MilesCranmer
Copy link
Member

Fixes #93 #94 by @jkrumbiegel.

All it turned out to be was a missing method

Base.similar(::QuantityArray, ::Type{<:Quantity}, ::Dims)

which is used in the standard lib for some of the array operations.

The method I had written had mistakenly assumed the ::Type here was the numeric eltype, like Float64. But in retrospect it was perhaps obvious that this would be the quantity type, because, after all, QuantityArray is an AbstractArray{Q,N} where Q is a quantity type.

Let me know if this works for you @jkrumbiegel


I also added a ::Q type assertion to prevent stack overflows related to converting a quantity of a quantity to a quantity.

We should probably implement a constructor for quantities of quantities that throws an error or simply converts to a regular quantity...

@SymbolicML SymbolicML deleted a comment from sweep-ai bot Nov 30, 2023
Copy link
Contributor

github-actions bot commented Nov 30, 2023

Benchmark Results

main d6864c5... t[main]/t[d6864c5...]
Quantity/creation/Quantity(x) 3.1 ± 0.01 ns 2.79 ± 0.01 ns 1.11
Quantity/creation/Quantity(x, length=y) 3.11 ± 0.001 ns 3.11 ± 0.001 ns 1
Quantity/with_numbers/*real 3.11 ± 0.01 ns 3.1 ± 0.01 ns 1
Quantity/with_numbers/^int 8.36 ± 2.5 ns 8.05 ± 2.2 ns 1.04
Quantity/with_numbers/^int * real 8.37 ± 2.5 ns 8.05 ± 2.2 ns 1.04
Quantity/with_quantity/+y 5.27 ± 0.01 ns 5.27 ± 0.01 ns 1
Quantity/with_quantity//y 3.11 ± 0.001 ns 3.41 ± 0.01 ns 0.912
Quantity/with_self/dimension 1.56 ± 0.01 ns 2.47 ± 0.91 ns 0.631
Quantity/with_self/inv 3.11 ± 0.01 ns 3.11 ± 0.01 ns 1
Quantity/with_self/ustrip 1.55 ± 0.01 ns 1.56 ± 0.01 ns 0.994
QuantityArray/broadcasting/multi_array_of_quantities 0.147 ± 0.14 ms 0.15 ± 0.012 ms 0.983
QuantityArray/broadcasting/multi_normal_array 0.059 ± 0.00019 ms 0.0529 ± 0.00023 ms 1.11
QuantityArray/broadcasting/multi_quantity_array 0.158 ± 0.00079 ms 0.159 ± 0.00099 ms 0.998
QuantityArray/broadcasting/x^2_array_of_quantities 26.4 ± 2.6 μs 26.6 ± 2.2 μs 0.994
QuantityArray/broadcasting/x^2_normal_array 4.95 ± 0.95 μs 4.71 ± 0.99 μs 1.05
QuantityArray/broadcasting/x^2_quantity_array 6.91 ± 0.27 μs 6.19 ± 0.36 μs 1.12
QuantityArray/broadcasting/x^4_array_of_quantities 0.0787 ± 0.00068 ms 0.0787 ± 0.00056 ms 0.999
QuantityArray/broadcasting/x^4_normal_array 0.0498 ± 0.0092 ms 0.0498 ± 0.0002 ms 1
QuantityArray/broadcasting/x^4_quantity_array 0.0592 ± 0.00018 ms 0.0592 ± 0.00018 ms 1
time_to_load 0.237 ± 0.0004 s 0.24 ± 0.0022 s 0.988

Benchmark Plots

A plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR.
Go to "Actions"->"Benchmark a pull request"->[the most recent run]->"Artifacts" (at the bottom).

@MilesCranmer
Copy link
Member Author

MilesCranmer commented Nov 30, 2023

One other issue I'm seeing is that

map(x -> x^2, QuantityArray([0.5u"m"]))

doesn't seem to work even with this change, due to a DimensionError (it creates a similar array without checking the output dimension). It seems we may need to overload _similar_for in the standard library, because it doesn't seem to rely on the broadcasting methods I've already overloaded.

@MilesCranmer
Copy link
Member Author

Fixed!

julia> qa = fill(1.5u"m", 3)
3-element QuantityArray(::Vector{Float64}, ::Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}):
 1.5 m
 1.5 m
 1.5 m

julia> map(x -> x^2, qa)
3-element QuantityArray(::Vector{Float64}, ::Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}):
 2.252.252.25

@MilesCranmer
Copy link
Member Author

I guess my one worry is whether calling first on an iterator would ever reset the start position for when you actually make use of the iterator... cc @gaurav-arya

@MilesCranmer MilesCranmer changed the base branch from main to fix-pow November 30, 2023 13:10
@MilesCranmer MilesCranmer force-pushed the fix-pow branch 2 times, most recently from 49a5165 to 1f2c1a5 Compare November 30, 2023 13:19
@MilesCranmer MilesCranmer force-pushed the MilesCranmer/issue94 branch 2 times, most recently from 8f87e2f to 13c7e4b Compare November 30, 2023 13:21
@MilesCranmer
Copy link
Member Author

Unittests done. Once you are content with this @jkrumbiegel we could merge this.

(Although I think I might need to add some extra unittests to get coverage on the other branches of _similar_for.)

Base automatically changed from fix-pow to main November 30, 2023 13:40
@MilesCranmer
Copy link
Member Author

Since this is a pretty nasty bug that the current errors are not useful for (stack overflow errors, etc.), I'm going to go ahead and merge now with a self-review. But let me know if anybody has comments and we can implement changes later!

@MilesCranmer MilesCranmer merged commit 344f4f4 into main Dec 2, 2023
8 checks passed
@MilesCranmer MilesCranmer deleted the MilesCranmer/issue94 branch December 2, 2023 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reduce(vcat returns corrupted result
1 participant