ModelingToolkit.jl icon indicating copy to clipboard operation
ModelingToolkit.jl copied to clipboard

does not support Unitful units

Open TorkelE opened this issue 10 months ago • 3 comments

I played around with Catalyst + Unitful (DynamicQuantities works well), but they cannot be combined. The problem is this function in ModelingToolkit, which assumes DQ type units:

function screen_unit(result)
    if result isa DQ.AbstractQuantity
        d = DQ.dimension(result)
        if d isa DQ.Dimensions
            if result != oneunit(result)
                throw(ValidationError("$result uses non SI unit. Please use SI unit only."))
            end
            return result
        elseif d isa DQ.SymbolicDimensions
            throw(ValidationError("$result uses SymbolicDimensions, please use `u\"m\"` to instantiate SI unit only."))
        else
            throw(ValidationError("$result doesn't use SI unit, please use `u\"m\"` to instantiate SI unit only."))
        end
    else
        throw(ValidationError("$result doesn't have any unit."))
    end
end

I am not sure what the planned extent of Unitful support is. If the idea is to fully support it, there might be a bug here. If not, then I guess it does not matter.

TorkelE avatar Apr 03 '24 23:04 TorkelE

If nothing else, the error messages could be made more informative :)

One thing here is that it looks like it's not actually testing whether it's SI units or not, it's testing whether the prefix is one or not. So for example, based on the code above it seems like I could make up my own non-SI units like in the example here, but as long as the prefix was 1, then it would be fine. Is that true?

ctessum avatar Aug 21 '24 22:08 ctessum

So to answer my own question, one can't use non-SI units because it tests for if d isa DQ.Dimensions, and custom-created units are not DQ.Dimensions, but they are a subtype of DQ.AbstractDimensions.

Would it work to have that line be if d isa DQ.AbstractDimensions instead?

ctessum avatar Aug 21 '24 23:08 ctessum

@YingboMa added the restriction to SI IIRC, and I'm not entirely sure why so I'll let him answer that.

ChrisRackauckas avatar Aug 22 '24 11:08 ChrisRackauckas