Incompatibility with DynamicQuantities.jl

[mikeingold]

Background

DynamicQuantities.jl defines Quantitys, like those in Unitful.jl, that are statically-typed to avoid issues like type instability. Instead of the dimensions of a Quantity being stored in the type-domain they're stored in the value-domain.

Issue

Meshes.jl doesn't currently compose with DynamicQuantities.jl because zero(::Type) for these Quantitys doesn't have access to the dimensions stored in the value (not in the type). This is mentioned in a currently-open Issue over there.

A similar Issue came up previously in composition of DynamicQuantities.jl and QuadGK.jl over implementation of one vs oneunit. It's not obvious to me whether the solution for this current issue would need to be implemented in which package (or both).

MWE

using DynamicQuantities
using Meshes

Point(1.0m, 2.0m)

Example error stack trace

ERROR: Cannot create an additive identity for a `UnionAbstractQuantity` type, as the dimensions are unknown. Please use `zero(::UnionAbstractQuantity)` instead.
Stacktrace:
 [1] error(s::String)
   @ Base .\error.jl:35
 [2] zero(::Type{DynamicQuantities.Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}})
   @ DynamicQuantities C:\Users\x\.julia\packages\DynamicQuantities\HYcKp\src\utils.jl:280
 [3] float(::Type{DynamicQuantities.Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}})
   @ Base .\float.jl:311
 [4] Vec(coords::Tuple{DynamicQuantities.Quantity{…}, DynamicQuantities.Quantity{…}})
   @ Meshes C:\Users\x\.julia\packages\Meshes\Lb9po\src\vectors.jl:46
 [5] Vec(::DynamicQuantities.Quantity{Float64, Dimensions{…}}, ::Vararg{DynamicQuantities.Quantity{…}})
   @ Meshes C:\Users\x\.julia\packages\Meshes\Lb9po\src\vectors.jl:58
 [6] Point(::DynamicQuantities.Quantity{Float64, Dimensions{…}}, ::Vararg{DynamicQuantities.Quantity{…}})
   @ Meshes C:\Users\x\.julia\packages\Meshes\Lb9po\src\primitives\point.jl:48
 [7] top-level scope
   @ REPL[15]:1
Some type information was truncated. Use `show(err)` to see complete types.

[MilesCranmer]

I think there are three potential solutions:

1. In Meshes.jl, use zero(x) which is more general than zero(typeof(x)). I think this is the easiest fix.
2. If the issue is getting the base numeric type, there is https://github.com/SymbolicML/BaseType.jl for extracting this. Some packages try to use typeof(one(float(typeof(...)))) for this purpose but it is not always guaranteed to work (like here).
3. We try some of the workarounds as described on that issue. However this is probably a more dangerous approach than 1-2 as it might introduce unexpected conversions.

Cheers,
Miles

[juliohm]

Thank you all for reporting the issue. We will take a look at it, and probably implement approach (1) as suggested.

@mikeingold do you have any comment regarding Unitful.jl versus DynamicQuantities.jl for coordinate types?

[mikeingold]

I just took another look through the stack trace. For the expression Point(1.0m, 2.0m) it looks like the snag actually begins at Meshes.jl/src/vectors.jl:46:

Vec(coords::NTuple{Dim,T}) where {Dim,T} = new{Dim,float(T)}(coords)

I took a quick look through Unitful to get a sense of how this works for that package. I didn't see a Base.float(::Type) method, but they implement Base.zero(::Type) at Unitful.jl/src/quantities.jl:364 and Base.float(x::AbstractQuantity) at Unitful.jl/src/quantities.jl:449.

[mikeingold]

@mikeingold do you have any comment regarding Unitful.jl versus DynamicQuantities.jl for coordinate types?

I’ve been using Unitful types for a while but DynamicQuantities seems like it has a better architecture for dimensional types moving forward. It’s getting more popular but has required some work in tracking down these kinds of compatibility issues.

[juliohm]

Do you have a specific reason to prefer DynamicQuantities.jl in the case of coordinates? Did you experience better performance with distance, area, or other geometric calculations?

[mikeingold]

I was going to link you to this Discourse thread for it but it looks like you've already seen it, so providing as reference here for anyone interested. I've been implementing it in some of my packages and seen some significant performance improvements.

I discovered this while testing a new testing package for composing Meshes with QuadGK to compute line integrals over 1-Dim polytopes. You can see a some usage examples in the runtests.jl where the Unitful versions work exactly as intended but I temporarily stopped testing with DynamicQuantities since it just errors out. With that working I think it would be a solid canonical test case for comparing performance between the two.

[mikeingold]

I don't see any real reason why the current Vec struct can't contain a DQ Quantity type, so this specific hurdle seems like it could be solved by just implementing another constructor. I'm not on a terminal at the moment, but maybe we could just add a modified method something like the following? Vec(coords::NTuple{Dim,T}) where {Dim,T<:AbstractQuantity} = new{Dim,T}(coords)

…

On Wed, Jan 17, 2024 at 11:40 AM Júlio Hoffimann ***@***.***> wrote: Do you have a specific reason to prefer DynamicQuantities.jl in the case of coordinates? Did you experience better performance with distance, area, or other geometric calculations? — Reply to this email directly, view it on GitHub <#688 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA657OVL6IRIY6CIZM4FQFDYO75HXAVCNFSM6AAAAABB6PU452VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJWGE4DSMJSGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[mikeingold]

Looks like the Point constructor just wraps a Vec, a similarly other types like the polytopes mostly just wrap Point's, so I don't see any obvious hurdles that we'd run into once the Vec{Dim,Quantity} is constructed. On Wed, Jan 17, 2024 at 2:02 PM Michael Ingold ***@***.***> wrote:

…

I don't see any real reason why the current Vec struct can't contain a DQ Quantity type, so this specific hurdle seems like it could be solved by just implementing another constructor. I'm not on a terminal at the moment, but maybe we could just add a modified method something like the following? Vec(coords::NTuple{Dim,T}) where {Dim,T<:AbstractQuantity} = new{Dim,T}(coords) On Wed, Jan 17, 2024 at 11:40 AM Júlio Hoffimann ***@***.***> wrote: > Do you have a specific reason to prefer DynamicQuantities.jl in the case > of coordinates? Did you experience better performance with distance, area, > or other geometric calculations? > > — > Reply to this email directly, view it on GitHub > <#688 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AA657OVL6IRIY6CIZM4FQFDYO75HXAVCNFSM6AAAAABB6PU452VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJWGE4DSMJSGE> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

[mikeingold]

I don't see any real reason why the current Vec struct can't contain a DQ Quantity type, so this specific hurdle seems like it could be solved by just implementing another constructor. I'm not on a terminal at the moment, but maybe we could just add a modified method something like the following? Vec(coords::NTuple{Dim,T}) where {Dim,T<:AbstractQuantity} = new{Dim,T}(coords)

@MilesCranmer Do you think this could be a good candidate for a DQ extension? I suspect that at least this specific issue could be addressed with something like the suggested Meshes.Vec constructor above, but this probably would need to be done in an extension in one of the packages. If so, I could take a crack at a PR to DQ adding it.

[MilesCranmer]

Yeah, sure!

[mikeingold]

Ok, that idea might have been a little too optimistic. I forked DQ and added an extension for Meshes. What I didn't consider was that it doesn't seem like we can access the special new constructor from the outside. Using new directly fails, but Vec{Dim,T}(coords) just defaults back to the previous constructor. Any ideas?

[MilesCranmer]

Using new directly fails, but Vec{Dim,T}(coords) just defaults back to the previous constructor.

This might be because UnionAbstractQuantity is at the same level in the type hierarchy as Number so there might be a method ambiguity... Does it work with AbstractQuantity?

[MilesCranmer]

If it does, then you could do

for (type, _, _) in ABSTRACT_QUANTITY_TYPES
    @eval Vec(coords::NTuple{Dim,T}) where {Dim,T<:$type} = Vec{Dim,T}(coords)
end

which is the same thing as doing <:UnionAbstractQuantity but it makes the methods more specific (so it gets in front of Number).

[mikeingold]

I tried again with the suggestion to reduce UnionAbstractQuantity to just AbstractQuantity and hit the same wall. Based on the stack traces I'm getting, the test is definitely invoking the Vec function defined in the extension, but then the right-hand-side Vec{Dim,T}(coords) is simply being captured by Meshes.Vec's single inner-constructor that still uses float(T). I'm not sure that it's even possible to circumvent inner-constructors, so trying to fix this with a DQ extension might not be possible without at least some change to Meshes.

[MilesCranmer]

Ah yeah you're right, this prevents it:

struct Vec{Dim,T} <: StaticVector{Dim,T}
  coords::NTuple{Dim,T}
  Vec(coords::NTuple{Dim,T}) where {Dim,T} = new{Dim,float(T)}(coords)
end

Actually one thing we can easily do in DynamicQuantities.jl is simply implement

float(::Type{Q}) where {T,D,Q<:AbstractQuantity{T,D}} = with_type_parameters(Q, float(T), D)

Since I believe this does not violate any rules about identity (which is why we can't safely define zero(::Type{<:AbstractQuantity{T}})).

Edit 2: actually this would work. Let me try again...

[mikeingold]

I actually just got a workaround implemented as a DQ extension in my fork here. It's a bit of a hack, as it circumvents the Meshes.Vec constructors entirely, but it might be at least good enough to proceed with testing in lieu of another solution. It currently passes all tests, including one for the MWE case above.

The "solution" I came up with was essentially to shadow Vec with a new binary-compatible struct with the exact same spec but no inner constructor, then reinterpret it from type to the other. Source here.

[MilesCranmer]

I think I might have found a way to fix the root cause here – SymbolicML/DynamicQuantities.jl#110 do you want to try this out?

It looks like float(::Type) by default will call zero(::Type), convert to a float, then call typeof. However, DynamicQuantities doesn't implement zero(::Type), so this fails.

But in principle there's no reason we can't just implement float(::Type) as the docstring simply asks for an equivalent floating-point version of the type. So the values of the dimensions wouldn't actually be an issue like they are for zero(::Type) (since zero has to create a value from a type, but float(::Type) just returns a type itself). Does that sort of make sense?

[mikeingold]

Well, no need for the hacky workaround. I just set up a temp environment with just Meshes and the meshes-compat branch of DynamicQuantities. All of the basic tests I'd done so far run without issue!

[MilesCranmer]

Awesome! Okay I'll merge that one then. Let me know if there are any other issues that come up

[mikeingold]

Based on this new development I’m marking this issue as closed. I’m planning to continue testing and integrating with these two packages and will let you guys know if I run into other snags.