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

Improve `@u_str` implementation to fix downstream precompilation #106

Closed MilesCranmer closed 5 months ago

MilesCranmer commented 6 months ago

Fixes #105 which reported precompilation issues.

github-actions[bot] commented 6 months ago

Benchmark Results

main 06365063fbfeab... t[main]/t[06365063fbfeab...]
Quantity/creation/Quantity(x) 3.1 ± 0.01 ns 3.41 ± 0.01 ns 0.911
Quantity/creation/Quantity(x, length=y) 3.11 ± 0.001 ns 3.11 ± 0.001 ns 1
Quantity/with_numbers/*real 3.1 ± 0.01 ns 3.1 ± 0.01 ns 1
Quantity/with_numbers/^int 8.67 ± 2.2 ns 8.05 ± 2.2 ns 1.08
Quantity/with_numbers/^int * real 8.37 ± 2.2 ns 8.67 ± 2.2 ns 0.965
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.42 ± 0.01 ns 1
Quantity/with_self/dimension 3.1 ± 0.01 ns 3.1 ± 0.01 ns 1
Quantity/with_self/inv 3.41 ± 0.01 ns 3.11 ± 0.001 ns 1.1
Quantity/with_self/ustrip 3.1 ± 0.01 ns 2.79 ± 0.01 ns 1.11
QuantityArray/broadcasting/multi_array_of_quantities 0.15 ± 0.001 ms 0.145 ± 0.0012 ms 1.04
QuantityArray/broadcasting/multi_normal_array 0.0499 ± 0.00026 ms 0.0529 ± 0.00019 ms 0.943
QuantityArray/broadcasting/multi_quantity_array 0.155 ± 0.0011 ms 0.155 ± 0.00065 ms 1
QuantityArray/broadcasting/x^2_array_of_quantities 25.7 ± 1.9 μs 25.9 ± 1.9 μs 0.993
QuantityArray/broadcasting/x^2_normal_array 5.42 ± 0.53 μs 5.56 ± 0.84 μs 0.974
QuantityArray/broadcasting/x^2_quantity_array 6.93 ± 0.25 μs 7.07 ± 0.29 μs 0.98
QuantityArray/broadcasting/x^4_array_of_quantities 0.0786 ± 0.00058 ms 0.0789 ± 0.00062 ms 0.996
QuantityArray/broadcasting/x^4_normal_array 0.0497 ± 0.00016 ms 0.0467 ± 0.00018 ms 1.06
QuantityArray/broadcasting/x^4_quantity_array 0.0499 ± 0.00019 ms 0.05 ± 0.00018 ms 0.998
time_to_load 0.132 ± 0.0012 s 0.135 ± 0.0017 s 0.981

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 commented 6 months ago

@YingboMa what do you think? This completely avoids any eval when evaluating u_str.

I also removed the limitation of type requirement. So you can do things like u"(m, s)".

@gaurav-arya are you okay with this? I think we might have to remove that type requirement to fix precompilation working in downstream packages, since evaluating types requires eval.

MilesCranmer commented 5 months ago

Since this is quite a significant change in the parsing I'm going to sit on this PR for a bit in case I think of anything wrong with it.

@gaurav-arya if you could take a look also that would be amazing

MilesCranmer commented 5 months ago

For backwards compatibility we could have as_quantity injected into the output – then one could still use u"1" to create quantities?

It's not 1:1 with Unitful then but at least it preserves the type stability. Like q = u"1"; q *= 1.5u"m" for example.

MilesCranmer commented 5 months ago

Okay I convinced myself the other way. I think u_str should always return the same type no matter what – it's just the safest thing to do. Even though macros are expanded during compilation, I can easily see the user encountering type instabilities because of this aforementioned issue, where u"1" returns a different type from u"m/m".

This is unlike Unitful, but is like the current implementation of DynamicQuantities. So with this change it's no longer breaking.

MilesCranmer commented 5 months ago

Woo! 100% test coverage!

Alright we're good now. I'm merging as it's no longer a breaking change. @gaurav-arya if you did have a comment on the behavior of u_str we can always implement the breaking change on a new version later.

@YingboMa please confirm that precompilation works on v0.11.2 and feel free to add to the stdlib now.