Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support dispatches on Number, Real, etc. #44

Closed
gaurav-arya opened this issue Sep 19, 2023 · 1 comment · Fixed by #49
Closed

Support dispatches on Number, Real, etc. #44

gaurav-arya opened this issue Sep 19, 2023 · 1 comment · Fixed by #49

Comments

@gaurav-arya
Copy link
Collaborator

gaurav-arya commented Sep 19, 2023

Inspired by #40 (comment):

It's too bad I can't do

abstract type AbstractQuantity{T,D} <: T end

which would make quantity containing a Number also a type Number.

While we can't do that, a practical solution could be to get this to work on a finite set of types T, e.g. Number and Real, Perhaps wrapper types could be a nice solution here, e.g. something like

struct NumberQuantityWrapper{Q} <: Number
    quantity::Q
end

struct RealQuantityWrapper{Q} <: Real
    quantity::Q
end

const QuantityWrapper = Union{NumberQuantityWrapper, RealQuantityWrapper}
ustrip(q::QuantityWrapper) = ustrip(q.quantity)
dimension(q::QuantityWrapper) = dimension(q.quantity)

function new_quantity(::Type{Q}, l, r) where {Q<:AbstractQuantity}
    quantity = constructor_of(Q)(l, r)
    if l isa Number 
        return NumberQuantityWrapper(quantity)
    elseif l isa Real
        return RealQuantityWrapper(quantity)
    else
        return quantity
    end
end

One difficulty here is that NumberQuantityWrapper and RealQuantityWrapper no longer live in the same type hierarchy as AbstractQuantity, but I think we could come up with a clean design using union types and/or metaprogramming that loops over the wrappers for easily defining dispatch rules on quantities. What do you think @MilesCranmer?

Addendum: Although in the wrapper type approach proposed here, AbstractQuantity would again be generic, I am in support of subtyping Number as in #40 as an initial step-- as long as it does not cause any concrete immediate issues, matching Unitful seems like the right thing. This issue is about having our cake and eating it too:) (Supporting a few carefully selected generic types like Real would hopefully go a long way to increasing composability of DynamicQuantities.jl even further, as compared to Unitful.jl.)

Edit: We also have to think about the consequences of breaking the AbstractQuantity hierarchy on possible extension rules in downstream packages. Perhaps the current AbstractQuantity should become AbstractGenericQuantity, and AbstractQuantity should be exported and equal to Union{AbstractGenericQuantity, QuantityWrapper}.

@MilesCranmer
Copy link
Member

A union type sounds like a smart approach to this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants