Skip to content

Fix numeric promotion rules #84

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

Merged
merged 3 commits into from
Nov 12, 2023
Merged

Fix numeric promotion rules #84

merged 3 commits into from
Nov 12, 2023

Conversation

MilesCranmer
Copy link
Member

@MilesCranmer MilesCranmer commented Nov 10, 2023

@gaurav-arya this should address your comment in #78.

Now you can do

[3.0, 1u"m"]

and it will be an array of Quantity{Float64,...}.

Copy link
Contributor

github-actions bot commented Nov 10, 2023

Benchmark Results

main 72b7c22... t[main]/t[72b7c22...]
Quantity/creation/Quantity(x) 2.9 ± 0.1 ns 3.2 ± 0.1 ns 0.906
Quantity/creation/Quantity(x, length=y) 3.3 ± 0.1 ns 3.7 ± 0.1 ns 0.892
Quantity/with_numbers/*real 3.3 ± 0.1 ns 3.3 ± 0.1 ns 1
Quantity/with_numbers/^int 10.1 ± 2.5 ns 10.1 ± 2.5 ns 1
Quantity/with_numbers/^int * real 10.5 ± 2.4 ns 10.5 ± 2.4 ns 1
Quantity/with_quantity/+y 6.6 ± 0.3 ns 5.9 ± 0.1 ns 1.12
Quantity/with_quantity//y 3.3 ± 0 ns 3.7 ± 0.1 ns 0.892
Quantity/with_self/dimension 1.6 ± 0.1 ns 1.6 ± 0.1 ns 1
Quantity/with_self/inv 3.3 ± 0.1 ns 3.7 ± 0.1 ns 0.892
Quantity/with_self/ustrip 1.6 ± 0.1 ns 1.6 ± 0.1 ns 1
QuantityArray/broadcasting/multi_array_of_quantities 0.221 ± 0.17 ms 0.197 ± 0.004 ms 1.12
QuantityArray/broadcasting/multi_normal_array 0.0683 ± 0.0017 ms 0.0682 ± 0.0009 ms 1
QuantityArray/broadcasting/multi_quantity_array 0.229 ± 0.0025 ms 0.229 ± 0.0032 ms 0.999
QuantityArray/broadcasting/x^2_array_of_quantities 0.0443 ± 0.004 ms 0.0434 ± 0.0038 ms 1.02
QuantityArray/broadcasting/x^2_normal_array 8.9 ± 1 μs 8.9 ± 1.1 μs 1
QuantityArray/broadcasting/x^2_quantity_array 11.2 ± 0.7 μs 10 ± 0.9 μs 1.12
QuantityArray/broadcasting/x^4_array_of_quantities 0.119 ± 0.0035 ms 0.118 ± 0.0042 ms 1
QuantityArray/broadcasting/x^4_normal_array 0.0585 ± 0.0007 ms 0.0585 ± 0.0012 ms 1
QuantityArray/broadcasting/x^4_quantity_array 0.082 ± 0.0014 ms 0.0852 ± 0.004 ms 0.962
time_to_load 0.205 ± 0.00045 s 0.215 ± 0.00072 s 0.953

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).

Copy link
Collaborator

@gaurav-arya gaurav-arya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not the biggest fan of having to manually list the numeric types here, as it's not extensible for other packages defining numeric-like types.

I think it deserves a bit of thought whether there's a slicker way to do this that still doesn't run into the issue you mentioned with other number packages. But I can't think about this at least for the next day or two, so if you'd like to merge and improve this looks good to me:)

@MilesCranmer
Copy link
Member Author

MilesCranmer commented Nov 10, 2023

Sounds good. Yeah I just have no idea how to figure out the ambiguity for things like

julia> using DynamicQuantities, Measurements

julia> [0.3±0.1, 3u"km/s"]
2-element Vector{Number}:
        0.3 ± 0.1
 3000.0 m s⁻¹

i.e., should this be a Measurement{Quantity} or a Quantity{Measurement}? (We could patch it for Measurements.jl, but what about the general case?) If we define a promotion rule over all of Number we would effectively be choosing Quantity{Measurement} which seems like undesired behavior. Wdyt?

And for extensibility, users can always define their own promotion_rule if needed

@MilesCranmer MilesCranmer merged commit cd30223 into main Nov 12, 2023
@MilesCranmer MilesCranmer deleted the numeric-promotion branch November 12, 2023 15:40
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.

2 participants