validate_upreferred() destroys performance. Should there be a way to opt out?

[RGonTheNoble]

I'm building a utility to convert arbitrary Units.jl objects into DynamicQuantities, becuase I can't use DynamicQuantities to handle affine units like Fahrenheit or psig. However, when converting these units, I ran into some serious performance issues. When looking at @profview results, nearly all the samples fell into some form of the validate_upreferred() function. Looking at how this function is implemented, it's easy to see why this would be slow, and why validating it for every single call would be cumbersome. Is there any way we can opt out of this behaviour?

https://github.com/SymbolicML/DynamicQuantities.jl/blob/f2bf5cb6b72f385cc7799e60879630da4c5f7570/ext/DynamicQuantitiesUnitfulExt.jl#L12

[MilesCranmer]

Oh yeah I guess it only needs to compute that result once. So I think just

+ @generated function validate_upreferred()
- function validate_upreferred()

would be all we need. Want to make a PR?

[MilesCranmer]

Edit:

-  function validate_upreferred()
+  @generated function validate_upreferred()
      si_units = get_si_units()
      for k in keys(si_units)
          if Unitful.upreferred(si_units[k]) !== si_units[k]
+             return :(error("Found custom `Unitful.preferunits`. This is not supported when interfacing Unitful and DynamicQuantities: you must leave the default `Unitful.upreferred`, which is the SI base units."))
-             error("Found custom `Unitful.preferunits`. This is not supported when interfacing Unitful and DynamicQuantities: you must leave the default `Unitful.upreferred`, which is the SI base units.")
          end
      end
      return true
  end

[RGonTheNoble]

I'll see what I can do. I temporarily disabled it on my end by redefining validate_upreferred() = true (bad Idea I know, but I wanted to see the performance improvements).

On a related note, I noticed a very slow dynamic dispatch on this line. https://github.com/SymbolicML/DynamicQuantities.jl/blob/f2bf5cb6b72f385cc7799e60879630da4c5f7570/ext/DynamicQuantitiesUnitfulExt.jl#L63

I think it's because dynamically calling keyword arguments is slow when running for loops because it can't doesn't trigger constant propagation. If we can create a function similar to the following:

function _raw_dimension(dim)
    dim_symbol = _map_dim_name_to_dynamic_units(typeof(dim))
    dim_power = dim.power
    return DynamicQuantities.Dimensions(R; dim_symbol => dim_power)
end

Mapping this to the the tuple D will trigger constant propagation.

[MilesCranmer]

That makes sense, sounds good to me.

[RGonTheNoble]

Okay, with a little bit of type-piracy (i.e. not even touching the source code) I replaced the conversion code with these two functions to remove _map_dim_name_to_dynamic_units and the dynamic dispatch it used:

function Base.convert(::Type{DynamicQuantities.Dimensions{R}}, d::Unitful.Dimensions{D}) where {R,D}
    validate_upreferred()
    return prod(Base.Fix1(_dynamic_dimension, R), D)
end

function _dynamic_dimension(::Type{R}, dim::Unitful.Dimension{D}) where {R,D}
    DimType = DynamicQuantities.Dimensions{R}
    D == :Length        && return DimType(length = dim.power)
    D == :Mass          && return DimType(mass = dim.power)
    D == :Time          && return DimType(time = dim.power)
    D == :Current       && return DimType(current = dim.power)
    D == :Temperature   && return DimType(temperature  = dim.power)
    D == :Luminosity    && return DimType(luminosity  = dim.power)
    D == :Amount        && return DimType(amount = dim.power)
    error("Unknown dimension: $D")
end

Original time 13.124057 seconds (37.98 M allocations: 1.230 GiB, 0.55% gc time)

Remove validate_upreferred() 0.910293 seconds (19.60 M allocations: 698.481 MiB, 4.37% gc time)

Remove dynamic dispatch from conversion 0.032560 seconds (6.87 k allocations: 100.519 MiB, 17.83% gc time)

[MilesCranmer]

Nice work!

[Deduction42]

I love @profview it makes this sort of thing pretty easy. I'll get on the pull request tomorrow. Probably from this account (I just realized I brought up the issue from my work account).

[Deduction42]

I made the changes but I can't publish a new branch. Are you able to give me permissions for this?

[MilesCranmer]

You need to fork the repo and use that fork to make a PR: https://docs.github.com/en/get-started/exploring-projects-on-github/contributing-to-a-project

[Deduction42]

I think I did it correctly (first time using this workflow). Let me know if you have any issues.

[Deduction42]

Okay, I realized I did some other changes that got loaded onto this branch. I reverted the changes to only the ones discussed on this thread (which is what I originally meant to do).

[RGonTheNoble]

I found a bug in this latest version. prod(Base.Fix1(_dynamic_dimension, R), D) fails when the units are dimensionless. An initializer needs to be supplied. prod(Base.Fix1(_dynamic_dimension, R), D, init=DynamicQuantities.Dimensions{R}())

[RGonTheNoble]

Are you able to hotfix this?

[MilesCranmer]

Hotfix in.

[Deduction42]

Thanks.