SymbolicML / DynamicQuantities.jl

Efficient and type-stable physical quantities in Julia
https://symbolicml.org/DynamicQuantities.jl/dev/
Apache License 2.0
120 stars 15 forks source link

Improved Base.isapprox support #114

Closed mikeingold closed 3 months ago

mikeingold commented 5 months ago

This PR is intended to address concerns discussed in #111, specifically:

This would technically be considered a breaking change due to the behavior change for atol keyword arguments. Previously, this argument took a ::Real and treated it as implicitly having the same units as the quantities being compared. As of this update, applicable units must be specified manually. This will make comparisons more explicit and less error prone. Additionally, this will enable comparisons like isapprox(1.234u"m", 1234.0u"cm"; atol=1.0u"mm").

github-actions[bot] commented 5 months ago

Benchmark Results

main 750b66e9d38934... main/750b66e9d38934...
Quantity/creation/Quantity(x) 3.41 ± 0.01 ns 2.79 ± 0 ns 1.22
Quantity/creation/Quantity(x, length=y) 3.11 ± 0.01 ns 3.42 ± 0.01 ns 0.909
Quantity/with_numbers/*real 3.1 ± 0.01 ns 3.11 ± 0.01 ns 1
Quantity/with_numbers/^int 8.05 ± 2.2 ns 8.37 ± 2.2 ns 0.963
Quantity/with_numbers/^int * real 8.67 ± 2.2 ns 8.05 ± 1.8 ns 1.08
Quantity/with_quantity/+y 4.04 ± 0.001 ns 4.04 ± 0.001 ns 1
Quantity/with_quantity//y 3.42 ± 0.01 ns 3.11 ± 0.001 ns 1.1
Quantity/with_self/dimension 3.1 ± 0.01 ns 2.79 ± 0.009 ns 1.11
Quantity/with_self/inv 3.11 ± 0.001 ns 3.11 ± 0.001 ns 1
Quantity/with_self/ustrip 2.79 ± 0.01 ns 2.79 ± 0.01 ns 1
QuantityArray/broadcasting/multi_array_of_quantities 0.145 ± 0.00092 ms 0.147 ± 0.00059 ms 0.986
QuantityArray/broadcasting/multi_normal_array 0.0529 ± 0.00019 ms 0.0499 ± 0.0002 ms 1.06
QuantityArray/broadcasting/multi_quantity_array 0.155 ± 0.00069 ms 0.155 ± 0.00064 ms 1
QuantityArray/broadcasting/x^2_array_of_quantities 26.1 ± 2.5 μs 27.2 ± 2.8 μs 0.959
QuantityArray/broadcasting/x^2_normal_array 5.3 ± 0.81 μs 5.72 ± 0.73 μs 0.926
QuantityArray/broadcasting/x^2_quantity_array 7.01 ± 0.3 μs 7.02 ± 0.26 μs 0.999
QuantityArray/broadcasting/x^4_array_of_quantities 0.0787 ± 0.00074 ms 0.079 ± 0.00067 ms 0.996
QuantityArray/broadcasting/x^4_normal_array 0.0467 ± 0.00018 ms 0.0499 ± 0.00019 ms 0.937
QuantityArray/broadcasting/x^4_quantity_array 0.0501 ± 0.00033 ms 0.05 ± 0.00018 ms 1
time_to_load 0.13 ± 0.00074 s 0.13 ± 0.0011 s 0.998

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

mikeingold commented 3 months ago

Sorry, I’ve been swamped the past few weeks. I’m going to try to make a final push on this PR in the coming days.

mikeingold commented 3 months ago

I just sync'd my fork with upstream main and merged all of the updates into this branch -- no conflicts.

mikeingold commented 3 months ago

I think this PR is in pretty good shape. Let me know if you have any more feedback that I can address.

Reminder: this PR would technically be considered breaking due to the change in atol behavior (requires dimensionful quantity to eliminate ambiguity and a potential foot-gun).

MilesCranmer commented 3 months ago

Thanks, will take a look now!

MilesCranmer commented 3 months ago

edit: resolved.

MilesCranmer commented 3 months ago

Edit: resolved

MilesCranmer commented 3 months ago

Okay I made some changes. Let me know what you think. Ideally we could also have tests for all the branches of code, such as atol/rtol set explicitly/left as default (with expected isapprox = true and = false) just so we know it works exactly as expected.

MilesCranmer commented 3 months ago

Okay I will implement the changes to avoid the per-component fallback. Give me a bit and I will declare when I'm done.

MilesCranmer commented 3 months ago

Ok, implemented. Let me know what you think.

mikeingold commented 3 months ago

Looks good to me. Nice work!

MilesCranmer commented 3 months ago

Thanks again for pushing this through!