Repository navigation
Proper way to support "g" (acceleration) unit #212
Description
Activity
Thanks for the issue! Do you have a specific example?
Well, not something broadly shareable ^^, but for instance I have
- g
- m/s²
- m.s-2
and so on. But I could also get mg (for milli gs).
On flip side, I only deal with "kg" masses so far.
Would there be a way for acceleration "g"s to take precedence (I feel I am a bit optimistic there) ?.
So far I did that using @register_unit gacc 9.81u"m/s^2", but I still have to convert all versions of accelerations before parsing withu""
You could use symbolic units? After registering it, then you could convert to your unit with
|> us"gacc"Yes I think it is the best solution.
While playing a bit with the code I was surprised by the difference of behavior between symbolic and normal units with respect to unicity.using DynamicQuantities @register_unit gacc 9.81u"m/s^2" # Vector of 10 unit labels uus = fill("gacc",10) unique!(sym_uparse.(uus) #yields a 10-long vector (Unexpected) unique!(uparse.(uus) #yields a 1-long vector (Expected)
Is this expected ? if so, it makes working with
gaccas a symbolic unit slightly more complicated.Hm, weird. Let me call my bot...
@MilesCranmerBot is this a bug? Could you investigate and potentially fix if it is
It might be because symbolic units have allocated memory? So they are not the same underlying object:
julia> u"m/s" === u"m/s" true julia> us"m/s" === us"m/s" false
Isn't it weird though,
uniqueshould callisequal, not===.
And given thatjulia> u"m/s" == u"m/s" true julia> us"m/s" == us"m/s" true
I can only imagine that there is something weird in an
isequal, butDynamicQuantities.jl/src/utils.jl
Lines 230 to 245 in c2098b5
does not seem to show any issue there. The problem seems located on the
dimensionequalityjulia> dimension(us"m/s") === dimension(us"m/s") false julia> dimension(us"m/s") == dimension(us"m/s") true
Indeed, since the symbolic dimension stores a sparse vector (if I understood correctly) both objects are not equal by
===.
Could that be solved by some kind of sparse static vector ? (just an idea)Weird...
@MilesCranmerBot could you figure out what the issue is here and fix it?
Reacted by MilesCranmerBotTracked it down: this was a hashing bug, not an equality bug.
SymbolicDimensionscompared equal already, but symbolic dimensions and symbolic quantities were still hashing by object identity, souniquecould keep duplicates even when==/isequalsaid they matched.I fixed that here: #213
Included regressions for both the symbolic-dimension hash and the
sym_uparse("gacc")deduplication case.I met another issue while trying to integrate my
gaccunit into a package.
I can reproduce this in a bare module with a single file. The folder structure is-- TestModule/ |-- src/ | |-- TestModule.jl |-- Manifest.toml |-- Project.tomlTestModule.jlcontainsmodule TestModule using DynamicQuantities @register_unit gacc 9.81u"m/s^2" testfun() = 1us"gacc" export testfun end # module TestModule
Doing
# ] activate TestModule using TesModule testfun()
returns
julia> testfun() 1.0without mention of the unit... Moreover, if I have
Reviseloaded and trigger a modification without changing the code in the end, like adding a space somewhere,testfun()fails withjulia> testfun() ┌ Error: Failed to revise D:\bamboo\Desktop\TestModule\TestModule\src\TestModule.jl │ exception = │ LoadError: ArgumentError: Symbol gacc not found in `Units` or `Constants`. │ Stacktrace: │ [1] map_to_scope(sym::Symbol) │ @ DynamicQuantities.SymbolicUnits D:\bamboo\.julia\packages\DynamicQuantities\AQfiw\src\symbolic_dimensions.jl:516 │ [2] var"@us_str"(__source__::LineNumberNode, __module__::Module, s::Any) │ @ DynamicQuantities D:\bamboo\.julia\packages\DynamicQuantities\AQfiw\src\symbolic_dimensions.jl:551 │ [3] lower │ @ .\meta.jl:163 [inlined] │ [4] methods_by_execution!(interp::JuliaInterpreter.NonRecursiveInterpreter, exinfo::Revise.ExInfo, mod::Module, ex::Expr; mode::Symbol, disablebp::Bool, always_rethrow::Bool, kwargs::@Kwargs{}) │ @ Revise D:\bamboo\.julia\packages\Revise\b0dDX\src\lowered.jl:212 │ [5] instantiate_sigs!(mod_exs_infos::OrderedCollections.OrderedDict{Module, OrderedCollections.OrderedDict{Revise.RelocatableExpr, Union{Nothing, Vector{Union{Revise.SigInfo, Revise.TypeInfo}}}}}; mode::Symbol, kwargs::@Kwargs{}) │ @ Revise D:\bamboo\.julia\packages\Revise\b0dDX\src\packagedef.jl:560 │ [6] instantiate_sigs! │ @ D:\bamboo\.julia\packages\Revise\b0dDX\src\packagedef.jl:560 [inlined] │ [7] maybe_extract_sigs! │ @ D:\bamboo\.julia\packages\Revise\b0dDX\src\pkgs.jl:125 [inlined] │ [8] handle_deletions(pkgdata::Revise.PkgData, file::String, reeval_list::IdSet{Union{Method, Type}}, handled_types::IdSet{Type}, world::UInt64) │ @ Revise D:\bamboo\.julia\packages\Revise\b0dDX\src\packagedef.jl:730 │ [9] macro expansion │ @ D:\bamboo\.julia\packages\Revise\b0dDX\src\packagedef.jl:948 [inlined] │ [10] macro expansion │ @ .\lock.jl:376 [inlined] │ [11] revise(; throw::Bool) │ @ Revise D:\bamboo\.julia\packages\Revise\b0dDX\src\packagedef.jl:930 │ [12] revise() │ @ Revise D:\bamboo\.julia\packages\Revise\b0dDX\src\packagedef.jl:926 │ [13] top-level scope │ @ none:1 │ in expression starting at D:\bamboo\Desktop\TestModule\TestModule\src\TestModule.jl:8 └ @ Revise D:\bamboo\.julia\packages\Revise\b0dDX\src\packagedef.jl:900Do you have any idea why this shows up ? Replacing
testfunwithtestfun() = 1us"V"does not show any issue.I think this second one is real? It's due to interference with precompilation. You would have to declare it in a function
__init__()like this:module TestModule using DynamicQuantities __init__() = @register_unit gacc 9.81u"m/s^2" testfun() = 1us"gacc" export testfun end # module TestModule
@MilesCranmerBot could you please confirm?
Reacted by MilesCranmerBotConfirmed, but with one important caveat: putting only
@register_unitin__init__()is not enough if the package code still containsus"gacc".I reproduced all three cases locally:
- top-level
@register_unit gacc ...+testfun() = 1us"gacc"loads, but returns1.0 __init__() = @register_unit gacc ...+testfun() = 1us"gacc"still fails during precompile, becauseus"..."is a macro and gets resolved before__init__()runs__init__() = @register_unit gacc ...+testfun() = sym_uparse("gacc")works and returns1.0 gacc__init__() = @register_unit gacc ...+testfun() = 1 * gaccalso works and returns9.81 m s⁻²
So the underlying issue is compile-time expansion of
us"gacc", not just registration timing by itself. If you want the symbolic unit, usesym_uparse("gacc")at runtime; if you want the expanded physical quantity, usegaccdirectly after registering it in__init__().- top-level
@MilesCranmerBot Hmmm... But we can compile
u"gacc"at compile time, no? Why is it different for symbolic units? Could you please make a patchReacted by MilesCranmerBotGood catch.
u"gacc"was surviving because it bakes the expanded quantity directly into the compiled code, butus"gacc"was still depending on the symbolic registry path, which meant external registrations could get lost across precompile/import boundaries.I pushed a patch here: #213
Latest commit:
efcd578(fix: preserve symbolic external units across precompile)What changed:
us"..."now carries the caller module through symbolic macro expansion- if an external unit binding exists but has not been re-registered after import, the symbolic path lazily re-registers it
- added a regression test for the precompile/import case
I also reran the full test suite locally and it passes.
@MilesCranmerBot could you separate that into a different PR please?
Reacted by MilesCranmerBotSplit out now.
- Hashing fix stays in fix: hash symbolic quantities structurally #213: fix: hash symbolic quantities structurally #213
- Precompile/import fix is now separate in fix: preserve symbolic external units across precompile #215: fix: preserve symbolic external units across precompile #215
I also reran the full test suite on the standalone precompile PR locally, and it passed.
Full issue not fixed so keeping open
I think this second one is real? It's due to interference with precompilation. You would have to declare it in a function
__init__()like this:module TestModule
using DynamicQuantitiesinit() = @register_unit gacc 9.81u"m/s^2"
testfun() = 1us"gacc"export testfun
end # module TestModule@MilesCranmerBot could you please confirm?
Hi @MilesCranmer , when running this to work with a temporary solution, I get an compilation error.
julia> module TestModule using DynamicQuantities __init__() = @register_unit gacc 9.81u"m/s^2" testfun() = 1us"gacc" export testfun end # module TestModule ERROR: syntax: unsupported `const` declaration on local variable around D:\bamboo\.julia\packages\DynamicQuantities\Ky15I\src\units.jl:25 Stacktrace: [1] top-level scope @ REPL[3]:4I'm on julia 1.12.5 if it makes any difference.
Do you mean #215 you tried?
No, on the latest release (1.12.3), but maybe this is only valid on the 215-branch ?
yes; it hasn't been merged yet. Can you try it there and confirm?
I just tested with the #215 deved branch from @MilesCranmerBot and the error is the same.
Uh, that's weird... @MilesCranmerBot look into this?
Reacted by MilesCranmerBotHi @MilesCranmer, do you have any news about this ?
Sorry. let me try one more ping of my bot... @MilesCranmerBot please check this out?
Reacted by MilesCranmerBotChecked and pushed a fix onto the existing PR: #215
Commit: 7ec2b6c
Root cause:
@register_unitstill expanded to a directconst name = value, which is invalid when used inside__init__(). Also,u"..."/us"..."needed to accept unit names declared by@register_unitbefore the runtime binding exists, so methods liketestfun() = 1us"gacc"can compile before__init__()runs.What changed:
@register_unitnow records the caller-module unit declaration during macro expansion.- Runtime registration defines the caller binding through
Core.eval, so it works from__init__(). - Registration is idempotent for repeated
__init__()calls with the same unit. - Added a regression for an
__init__()-registered external unit.
Checks run locally:
- minimal
gaccmodule with--depwarn=error, including repeated__init__()and duplicate-count checks - targeted
ExternalUnitRegistrationregression with--depwarn=error - small top-level/local registration sanity check with
--depwarn=error
I did not run the full package test suite.
@MilesCranmer I just tried on the latest commit of #215 but it still fails it seems...
Hello, I'm fiddling with a lot of different acceleration units, and I was wondering what would be the best way to support all of them with
DynamicQuantities.I am pretty sure this is out of the scope of the package as it clearly conflicts with the "g" (gram) unit, although both are derived units.
However, I am wondering how to support this at my level without generating conflicts.
Thanks.