Unitful conversion fails for symbolic units

[jkrumbiegel]

julia> convert(Unitful.Quantity, DynamicQuantities.u"s")
1.0 s

julia> convert(Unitful.Quantity, DynamicQuantities.us"s")
ERROR: type NamedTuple has no field s
Stacktrace:
 [1] getindex
   @ ./namedtuple.jl:137 [inlined]
 [2] convert(#unused#::Type{Quantity}, x::DynamicQuantities.Quantity{Float64, DynamicQuantities.SymbolicDimensions{DynamicQuantities.FixedRational{Int32, 25200}}})
   @ DynamicQuantitiesUnitfulExt ~/.julia/packages/DynamicQuantities/81OKc/ext/DynamicQuantitiesUnitfulExt.jl:35
 [3] top-level scope
   @ REPL[66]:1

[MilesCranmer]

Thanks. I guess it just needs a uexpand if it's a SymbolicDimensions input?

[jkrumbiegel]

Yes that's what I did as well. I wasn't sure if that would be fit for the "official" implementation due to the changes in numbers when converting to non symbolic

[MilesCranmer]

I think it would be perhaps too complicated to test that all ~120 or so symbols map to the correct Unitful versions, so despite this indeed changing the numerical value, I think it’s okay. Also the conversion utility was made before the units module was added here, I think it has less use now as you can just do everything in DynamicQuantities. Wdyt?

[jkrumbiegel]

I'm using this for testing that results match our previous Unitful implementation, but they already don't 100% match because once in a while floating point results change slightly. I guess that is expected when different instructions are being used sometimes by the compiler. So as I'm only asserting approximate equality, the conversion to non-symbolic seems not to matter in practice, my tests at least still pass. Maybe then this should just throw an error and explain what's going on, instead of the bug that you get now.

[MilesCranmer]

I see, that makes sense.

By error do you mean the conversion step should give an error on symbolic dimensions input?

[jkrumbiegel]

Yes, this could just mention that conversion is only defined for non-symbolic, and mention uexpand:

julia> convert(Unitful.Quantity, DynamicQuantities.us"s")
ERROR: type NamedTuple has no field s

[MilesCranmer]

That sounds good to me. If I can find some time soon I can add it, but someone else who is interested could add it and I can merge.