Allow External Unit Registration

[ven-k]

• By interpolating Units.UNIT_SYMBOLS while registering units and updating vectors-and-maps-of-units, new units can be registered even from outside the Units module
• Internally, units are registered lazily with _lazy_register_unit aka
  UNIT_MAPPING is updated after all units are defined.

Fixes:

julia> DynamicQuantities.Units.@register_unit Wb "m^2*kg*s^-2*A^-1"
ERROR: UndefVarError: `_UNIT_SYMBOLS` not defined
Stacktrace:
 [1] top-level scope
   @ C:\Users\user\.julia\packages\DynamicQuantities\5QflN\src\units.jl:25

[github-actions]

Benchmark Results

	main	92df2f4...	t[main]/t[92df2f4...]
Quantity/creation/Quantity(x)	2.79 ± 0.009 ns	3.11 ± 0.01 ns	0.9
Quantity/creation/Quantity(x, length=y)	3.41 ± 0.01 ns	3.11 ± 0.001 ns	1.1
Quantity/with_numbers/*real	3.11 ± 0.01 ns	3.11 ± 0.01 ns	1
Quantity/with_numbers/^int	8.37 ± 2.2 ns	8.67 ± 2.2 ns	0.965
Quantity/with_numbers/^int * real	8.05 ± 1.9 ns	8.37 ± 2.2 ns	0.963
Quantity/with_quantity/+y	4.04 ± 0.001 ns	4.04 ± 0.001 ns	1
Quantity/with_quantity//y	3.11 ± 0.001 ns	3.42 ± 0.01 ns	0.909
Quantity/with_self/dimension	2.79 ± 0.009 ns	3.1 ± 0.01 ns	0.9
Quantity/with_self/inv	3.11 ± 0.001 ns	3.41 ± 0.01 ns	0.912
Quantity/with_self/ustrip	2.79 ± 0.01 ns	3.1 ± 0.01 ns	0.903
QuantityArray/broadcasting/multi_array_of_quantities	0.151 ± 0.0011 ms	0.149 ± 0.0011 ms	1.01
QuantityArray/broadcasting/multi_normal_array	0.0499 ± 0.00018 ms	0.0499 ± 0.00018 ms	1
QuantityArray/broadcasting/multi_quantity_array	0.155 ± 0.00072 ms	0.155 ± 0.00071 ms	1
QuantityArray/broadcasting/x^2_array_of_quantities	25.3 ± 1.9 μs	25.9 ± 1.7 μs	0.978
QuantityArray/broadcasting/x^2_normal_array	5.54 ± 0.91 μs	5.86 ± 0.48 μs	0.945
QuantityArray/broadcasting/x^2_quantity_array	7 ± 0.25 μs	7.01 ± 0.27 μs	0.999
QuantityArray/broadcasting/x^4_array_of_quantities	0.0787 ± 0.00052 ms	0.0788 ± 0.00054 ms	0.999
QuantityArray/broadcasting/x^4_normal_array	0.0499 ± 0.00019 ms	0.0499 ± 0.00017 ms	1
QuantityArray/broadcasting/x^4_quantity_array	0.05 ± 0.00017 ms	0.05 ± 0.00017 ms	1
time_to_load	0.131 ± 0.00075 s	0.137 ± 0.0015 s	0.958

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]

Great idea! Can you rebase to #106? As there are changes which might affect the behavior.

[MilesCranmer]

Thanks! Quick initial comment is that this will need to update symbolic_dimensions.jl's ALL_VALUES to be vectors as well, so that new units are also included in symbolic units (like us"km/h").

[MilesCranmer]

One other quick thought is that we will probably need to change INDEX_TYPE to UInt16 in symbolic_dimensions.jl. I could see that if we are to allow users to add units and constants, the 8-bit indexing will probably be closer to overflow. However this also probably means things will be a bit slower.

[MilesCranmer]

Let me know when you're ready and I can give more comments or help out

[ChrisRackauckas]

@ven-k ?

[ven-k]

Now works:

julia> using ModelingToolkit, DynamicQuantities

julia> @register_unit Wb u"kg*m^2*s^−2*A^−1"

julia> @variables k [unit = u"Wb"]
1-element Vector{Num}:
 k

[MilesCranmer]

Looking good. I added a few suggestions.

[MilesCranmer]

One other comment: trying to register the same unit twice should have no effect. That way you could register units within a method scope without fear of creating duplicates. Also it would probably make things safer for Revise.jl

[ven-k]

trying to register the same unit twice should have no effect

Just comparing UNIT_VALUE[index].value == value, here, (aka user input value) will not work as latter is an Expr. For running this check, is there an internal helper which can expand it to its intended form?

[MilesCranmer]

Thanks for implementing all of those changes. Sorry to go back on what @devmotion said but do you think we could avoid the OrderedSet use? Since ALL_VALUES is a vector I think ALL_SYMBOLS should be as well – otherwise I get worried there could be an indexing bug introduced somewhere. If not now, maybe later. But I think this is actually already buggy because ALL_SYMBOLS could be left unchanged (since its a set) but ALL_VALUES could have another element added to it – since the function doesn't return early.

Basically I think those constants should just be write once read many arrays. When you push! to the ALL_SYMBOLS within the register units part, you could simply have a check to see if the symbol is already in it – if so, return early.

(Also, the fewer deps the better)

I think the struct could just be something like

struct WriteOnceReadManyVector{T}
    _raw_data::Vector{T}
end

and just define Base.push! and only the methods needed but nothing else (so the array can't be removed from). You could also do the same for WriteOnceReadManyDict for ALL_MAPPING also I think.

[MilesCranmer]

Now that I think about it, trying to register a unit should trigger an error if the symbol is already used AND the value assigned to that symbol is different than the one being registered.

But if the symbol is used already and the value is the same, it could simply be ignored and the unit registration should return early (before pushing anything).

[ven-k]

I've added tests for precompilation of register_unit in a closed module.
And updating SciML/ModelingToolkitStandardLibrary.jl#214 with new units¹, works too.

Footnotes

1. https://github.com/SciML/ModelingToolkitStandardLibrary.jl/pull/214/commits/17dc7cb4bf756db95268bf1c7454c4e6c4207f98 ↩

[MilesCranmer]

Looking good. Quick comment is I wonder if you could move the type definition of WriteOnceReadMany as well as its methods into a separate file, rather than mix them with Quantity? e.g., you will see that FixedRational, another internal type, gets its own file.

[ven-k]

I expected types.jl to house all types; now I've moved WriteOnceReadMany to its own file with this 5c00b03.

[MilesCranmer]

Thanks!

(Currently types.jl is supposed to be “exported types” while internal types live in separate files. However I haven’t been completely consistent on this and a future refactor would certainly improve readability.)

[MilesCranmer]

Cool. We're back at 100% test coverage. I did a deep review + some edits and everything is looking good now! Making some final tweaks and then will merge.

Thanks so much for the great contribution!!

[MilesCranmer]

One thing I'm confused about is that the @register_unit does not seem to propagate to other modules. e.g., if I register a unit in ExternalUnitRegistration.jl, and load it with using ExternalUnitRegistration, I can't use it in my current module via u"Wb"... Why is that? Is that intended?

Super weird:

julia> ExternalUnitRegistration.ALL_SYMBOLS
DynamicQuantities.WriteOnceReadMany{Vector{Symbol}}([:m, :g, :s, :A, :K, :cd, :mol, :fm, :pm, :nm  …  :L_sun, :L_bol0, :sigma_T, :au, :pc, :ly, :atm, :kpc, :Mpc, :Gpc])

Where is the :Wb??

Is this some weird thing about precompilation or maybe LOAD_PATH? If I define a module in my REPL it works just fine...

[MilesCranmer]

Everything looks good to me. I'm just super confused about the following behaviour:

julia> push!(LOAD_PATH, joinpath(@__DIR__, "test", "precompile_test"))
4-element Vector{String}:
 "@"
 "@v#.#"
 "@stdlib"
 "/Users/mcranmer/PermaDocuments/DynamicQuantities.jl/test/precompile_test"

julia> using ExternalUnitRegistration

julia> ExternalUnitRegistration.ALL_SYMBOLS
DynamicQuantities.WriteOnceReadMany{Vector{Symbol}}([:m, :g, :s, :A, :K, :cd, :mol, :fm, :pm, :nm  …  :L_sun, :L_bol0, :sigma_T, :au, :pc, :ly, :atm, :kpc, :Mpc, :Gpc])

Where is :Wb? Did the symbol list somehow get reset?

---

Maybe this is because I'm using Revise.jl?

[ven-k]

That's an expected behavior (even without Revise).

So currently the externally defined units can be used with u"unitname" within the scope of the module (and its sub-modules by importing). Outside they should be treated as a var. However it can be used, divided, multiplied as any other unit (but without u"").

Ex: @test Wb/u"m^2*kg*s^-2*A^-1" == 1.0 here¹

Footnotes

1. https://github.com/SymbolicML/DynamicQuantities.jl/pull/107/files#diff-97287d1f47874f9eb568bc53640b753ce7f6cd5fd70a18180a4beb9dd8a7939bR1895 ↩

[MilesCranmer]

Got it! I like that better actually, feels safer.