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 AbstractRealQuantity #85

Merged
merged 39 commits into from
Nov 23, 2023
Merged

Add AbstractRealQuantity #85

merged 39 commits into from
Nov 23, 2023

Conversation

MilesCranmer
Copy link
Member

@MilesCranmer MilesCranmer commented Nov 13, 2023

This adds a new AbstractRealQuantity <: Real and a concrete type RealQuantity. Users can wrap quantities with RealQuantity(0.3u"km/s") to pass to functions that require real input. But the default would still be Quantity <: Number for reasons discussed here.

I also introduce a new function promote_quantity which one can overload as a sort of custom type hierarchy, so that RealQuantity -> Quantity if it interacts with a non-real, and {RealQuantity,Quantity} -> GenericQuantity if they interact with non-numeric.

However this is a bit of a mess right now due to promotion complexities, making (0.3+2im)u"km/s" promote to a Complex{<:Quantity} instead of a Quantity... Anybody interested in having a go? Fixed!

Copy link
Contributor

github-actions bot commented Nov 13, 2023

Benchmark Results

main 0609463... t[main]/t[0609463...]
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.01 ns 3.11 ± 0.01 ns 1
Quantity/with_numbers/*real 3.1 ± 0.01 ns 3.1 ± 0.01 ns 1
Quantity/with_numbers/^int 8.37 ± 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.55 ± 0.01 ns 1.55 ± 0.01 ns 1
Quantity/with_self/inv 3.11 ± 0.001 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.016 ms 0.15 ± 0.14 ms 0.985
QuantityArray/broadcasting/multi_normal_array 0.0499 ± 0.003 ms 0.053 ± 0.00026 ms 0.941
QuantityArray/broadcasting/multi_quantity_array 0.158 ± 0.00092 ms 0.158 ± 0.00072 ms 0.999
QuantityArray/broadcasting/x^2_array_of_quantities 26.5 ± 3.3 μs 27.2 ± 1.9 μs 0.976
QuantityArray/broadcasting/x^2_normal_array 4.35 ± 0.85 μs 5.19 ± 0.83 μs 0.838
QuantityArray/broadcasting/x^2_quantity_array 6.97 ± 0.27 μs 6.03 ± 0.4 μs 1.16
QuantityArray/broadcasting/x^4_array_of_quantities 0.0788 ± 0.00078 ms 0.0787 ± 0.00055 ms 1
QuantityArray/broadcasting/x^4_normal_array 0.0498 ± 0.00015 ms 0.0498 ± 0.00018 ms 0.998
QuantityArray/broadcasting/x^4_quantity_array 0.0592 ± 0.00016 ms 0.0593 ± 0.00019 ms 0.998
time_to_load 0.162 ± 0.006 s 0.238 ± 0.0058 s 0.681

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

Actually I think we shouldn't make all default quantities Real. We can just have these as quantities that someone can use if they want that particular dispatch.

@MilesCranmer MilesCranmer changed the title [WIP] Add a new AbstractRealQuantity Add a new AbstractRealQuantity Nov 21, 2023
@MilesCranmer
Copy link
Member Author

@gaurav-arya I'm wondering if we should just make Quantity <: Real, and NumberQuantity <: Number. Having the default numeric type be RealQuantity rather than Quantity could be a bit awkward...

@MilesCranmer MilesCranmer changed the title Add a new AbstractRealQuantity Add AbstractRealQuantity Nov 21, 2023
src/types.jl Outdated Show resolved Hide resolved
@gaurav-arya
Copy link
Collaborator

Actually I think we shouldn't make all default quantities Real. We can just have these as quantities that someone can use if they want that particular dispatch.

Why not choose the most specific type given the value type? E.g. 1.0 * u"m" would be <: Real but (1.0 + 1.0 * im) * u"m" would be <: Number for now, etc. Is there a disadvantage, i.e. a reason the user would have preferred Quantity{<:Real} as opposed to RealQuantity{<:Real}?

In this scenario, I wonder if we even need a single DEFAULT_QUANTITY_TYPE? Instead, we have a default mapping from value type -> quantity type?

@MilesCranmer
Copy link
Member Author

MilesCranmer commented Nov 21, 2023

Why not choose the most specific type given the value type? E.g. 1.0 * u"m" would be <: Real but (1.0 + 1.0 * im) * u"m" would be <: Number for now, etc. Is there a disadvantage, i.e. a reason the user would have preferred Quantity{<:Real} as opposed to RealQuantity{<:Real}?

Yeah good point. The current behavior in this PR is actually (1.0 * u"m")::Real and ((1.0 + 1.0 * im) * u"m")::Number already. But I'm wondering if we should change this...

In this scenario, I wonder if we even need a single DEFAULT_QUANTITY_TYPE? Instead, we have a default mapping from value type -> quantity type?

Maybe do you mean like #66? Although I don't think we necessarily need an AbstractFloatQuantity yet, unless a specific need arises, since it will increase load time. But AbstractRealQuantity seems necessary for many packages.

@MilesCranmer
Copy link
Member Author

MilesCranmer commented Nov 21, 2023

Also:

Why not choose the most specific type given the value type? E.g. 1.0 * u"m" would be <: Real

Is this semantically correct? One could indeed argue that 1.0 meters is a type of number... But is it a real?

Let me give an example. Would you agree with the statement

$(0.5 \text{ km} \text{ s}^{-1}) \in \mathbb{R}$

? I think it kind of seems incorrect... It's more like:

$(0.5 \text{ km} \text{ s}^{-1}) \in \mathbb{R} \times \mathbb{U}$

for some abstract space $\mathbb{U}$ of physical dimensions. In which case it might not be the best option to set the default type as ::Real.

Whereas Number is much more abstract and I could totally say that a physical quantity is a type of number. i.e., $\mathbb{R} \times \mathbb{U}$ is a space of "numbers."

This semantic ambiguity leads me to want to keep Quantity <: Number as the default, but allow RealQuantity as a wrapper for users who know what they are doing. Wdyt?

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.

Left some small comments! FYI this has been a bit of a "drive by" review, I'm a little swamped with my semester. But the overall idea I'm totally in support of.

Regarding aggressively defaulting to <: Real: point taken that it might require more thought, can easily be a separate PR (which we could arguably say is non-breaking, while going the other way around and reverting the aggressive default would deifnitely be breaking). One counterpoint is that ForwardDiff.Dual <: Real -- I would be interested in seeing if there's any practical use case where a quantity breaks the expected semantics of Real. (Edit: I see there's some interesting discussion about this in PainterQubits/Unitful.jl#680)

Edit: one more thing -- conditional on the default type definition remaining, I do agree with the current PR decision to keep it as Number rather than Real.

src/arrays.jl Show resolved Hide resolved
src/disambiguities.jl Show resolved Hide resolved
src/disambiguities.jl Outdated Show resolved Hide resolved
src/math.jl Outdated Show resolved Hide resolved
@MilesCranmer
Copy link
Member Author

Thanks!

@MilesCranmer MilesCranmer merged commit b27ed80 into main Nov 23, 2023
7 checks passed
@MilesCranmer MilesCranmer deleted the real-quantity-2 branch November 23, 2023 18:18
@MilesCranmer MilesCranmer restored the real-quantity-2 branch November 23, 2023 18:25
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