Meshes.jl test failure

[MilesCranmer]

Not sure what this error is from... @mikeingold any idea?

julia> using DynamicQuantities, Meshes

julia> v = Meshes.Vec(1.0u"m/s", 2.0u"m/s", 3.0u"m/s")
ERROR: StackOverflowError:

This worked fine like a couple of weeks ago (included in test/test_meshes.jl). And DynamicQuantities.jl doesn't implement any special code for Meshes.jl.

If I pass a tuple instead it gives me more info:

julia> v = Meshes.Vec((1.0u"m/s", 2.0u"m/s", 3.0u"m/s"))
ERROR: StackOverflowError:
Stacktrace:
 [1] ntuple
   @ ./ntuple.jl:50 [inlined]
 [2] copy
   @ ./broadcast.jl:1097 [inlined]
 [3] materialize
   @ ./broadcast.jl:867 [inlined]
 [4] Vec(coords::Tuple{Quantity{Float64, Dimensions{…}}, Quantity{Float64, Dimensions{…}}, Quantity{Float64, Dimensions{…}}}) (repeats 58052 times)
   @ Meshes ~/.julia/packages/Meshes/BNwtP/src/vectors.jl:62
Some type information was truncated. Use `show(err)` to see complete types.

@juliohm @eliascarv do you know why this might be? Are there any breaking changes recently that might have changed this syntax?

[MilesCranmer]

Ah, okay, it seems like they changed the definition two weeks ago so that Vec takes as input only AbstractFloat or Unitful.Quantity, whereas before it was Any

JuliaGeometry/Meshes.jl@8c9b780

+ const Continuous = Union{AbstractFloat,Quantity{<:AbstractFloat}}

...

- Vec{Dim,T}(coords...) where {Dim,T} = Vec{Dim,T}(coords)
+ Vec{Dim,T}(coords...) where {Dim,T<:Continuous} = Vec{Dim,T}(coords)

Can this breaking change be rolled back? It would significantly better if this Continuous was simply a trait defined by ScientificTypesBase.jl rather than this rigid constant with Unitful support patched in at compile time (and therefore no DynamicQuantities support allowed)

[mikeingold]

Meshes.jl is going through an overhaul to their Coordinate Reference Systems, which has included a number of breaking changes. I've done some advocating for DynamicQuantities.jl compatibility, but it sounds like right now their focus is on just getting the biggest changes live first.

[MilesCranmer]

I see, thanks.

I think this is really just a bit of a misstep in implementing the following restriction:

struct Vec{Dim,T<:Union{AbstractFloat,Unitful.Quantity{<:AbstractFloat}}} <: StaticVector{Dim,T}

Before it was the more flexible T. It looks like this was done to simplify the conversion of Int64 into Float64... But seems a bit overkill and loses all modularity. Why not just have special behavior according to some trait that maps ::Integer into floats, and doesn't touch other stuff?

You ideally would want this to allow any wrapper of an AbstractFloat, instead of only one package.

[MilesCranmer]

Ping @juliohm @eliascarv just so you are aware. Is there any way Meshes.jl could keep compatibility with DynamicQuantities.jl?

[juliohm]

@MilesCranmer we are moving our design to compile-time coordinates with Unitful.jl, so no plans to support DynamicQuantities.jl at present. We leverage compile-time features to dispatch appropriate coordinate conversion methods.

Given the large number of points in industrial applications, we cannot afford representing units with values. They are compiled away as type parameters in our structs.

[MilesCranmer]

I am a bit confused by your message.

By leveraging dispatch do you mean you define methods for individual units yourself?

Why not leave the choice of Unitful/DynamicQuantities/other up to the user? I feel like you shouldn’t have any internal code that even is aware about units; Julia should just take care of it. Generic code is always better in Julia; if you need to have special behavior for different types, that is perhaps a code smell that suggests a refactor is needed.

It’s not generally true that “speed is improved by putting units in the type”. Actually as the example on the README shows it can be the opposite. And also do read Chris’s explanation for why value-based units can often result in the same speed even in the type stable regime: https://discourse.julialang.org/t/dynamicquantities-jl-v0-7-0-efficient-and-type-stable-physical-quantities/103709/6?u=milescranmer

^This is why ModelingToolkit.jl is transitioning from Unitful to DynamicQuantities.

[juliohm]

If you can propose an alternative design in CoordRefSystems.jl that is as cpu-efficient and as memory-efficient with DynamicQuantities.jl we can consider taking a look into it: https://github.com/JuliaEarth/CoordRefSystems.jl

We provide conversion methods with dispatch, and enforce unit conversion at construction time.

[MilesCranmer]

The easiest thing to try would just be

- using Unitful
+ using DynamicQuantities

within CoordRefSystems.jl and see if it works. Much of the API is identical. The main tweak needed is whether you need QuantityArray or not.

But it will result in faster startup times and sometimes much faster run times. In the type stable regime, the removal of units might not even buy you much performance (see Chris’s post).

It’s also good for user code, because users don’t often realize that a runtime snippet flag ? x^2 : x will literally be 1000x slower than the compile time snippet flag === Val(true) ? x^2 : x in Unitful. So for user-facing packages I think DQ makes more sense.

I think @mikeingold also mentioned seeing faster speeds with DQ in his stuff which is close in subject to Meshes.

[juliohm]

It would be interesting to see this PR where we simply switch from Unitful.jl to DynamicQuantities.jl and everything just works in CoordRefSystems.jl That would give us evidence that we can be agnostic to the unit types.

I remember that we have some non-trivial dispatch happening, and needed the exact types from Unitful.jl to make it work unambiguously. Happy to be proved wrong.

[MilesCranmer]

I remember that we have some non-trivial dispatch happening

This is the perfect use-case for DQ 😉

Anyways I don't have enough of a stake to make a PR but maybe @mikeingold could try if he is interested.