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

Equivalence involving nondimensional quantity #6

Closed
rmsrosa opened this issue Nov 13, 2020 · 7 comments · Fixed by #7
Closed

Equivalence involving nondimensional quantity #6

rmsrosa opened this issue Nov 13, 2020 · 7 comments · Fixed by #7

Comments

@rmsrosa
Copy link
Contributor

rmsrosa commented Nov 13, 2020

In water treatment, for example, it is common to treat mg/l as ppm. Currently, UnitfulEquivalences.jl does not allow equivalences involving nondimensional quantities such as concentration. I managed to do that with two dispatches of edconvert:

@equivalence DensityConcentration

function edconvert(d::dimtype(Unitful.Density), x::Unitful.Quantity{T,D,U}, e::DensityConcentration) where {T,D,U}
    D == NoDims ? x * 1u"kg/L" : throw(_eqconversion_error(d, D, e))
end

function edconvert(d::Unitful.Dimensions{()}, x::Unitful.Quantity{T,D,U}, e::DensityConcentration) where {T,D,U}
    D == Unitful.𝐌/Unitful.𝐋^3 ? x * 1u"L/kg" : throw(_eqconversion_error(d, D, e))
end

But since it is a direct relation, it would be nice to be able to define them at once with something like

@equivalence DensityConcentration
@eqrelation DensityConcentration Unitful.Density / Unitful.NoDims = 1u"kg/l"

I extended dimtype with dimtype(::Unitful.Dimensions{()}) = typeof(Unitful.NoDims) (despite the type piracy warning) so it could handle nondimensional quantities, but it was not enough since apparently edrelation also complains about Unitful.NoDims as the second argument. I think this is also related to the fact that I had to use Unitful.Quantity{T,D,U} in my first edconvert solution above.

As for the second edconvert, the following code also works for the conversion from Density to NoDims, without the need for Unitful.Quantity{T,D,U}, but for some reason that I do not understand it also gives me a warning of type piracy:

edconvert(d::Unitful.Dimensions{()}, x::Unitful.Density, e::DensityConcentration) = x * 1u"L/kg"

So I guess this is a "feature request" to allow one nondimensional quantity in @eqrelation.

@sostock
Copy link
Owner

sostock commented Nov 13, 2020

I have never seen a “type piracy warning”, can you elaborate on that?

The first edconvert methods can be simplified as follows:

edconvert(::dimtype(Unitful.Density), x::DimensionlessQuantity, e::DensityConcentration) = x * 1u"kg/L"

But I agree that this should work with the macro. I will implement it so that @eqrelation can be used with DimensionlessQuantity.

@sostock
Copy link
Owner

sostock commented Nov 13, 2020

#7 implements the necessary dimtype method.

One thing that is still missing for dimensionless quantities to work correctly is support for plain numbers: uconvert(u"kg/L", 5u"mm/m", DensityConcentration()) works, but uconvert(u"kg/L", 5, DensityConcentration()) does not. I will implement that as well. Note that the conversion to plain numbers already works: uconvert(NoUnits, 5u"kg/L", DensityConcentration()) Support for plain numbers is now included in #7 as well.

@rmsrosa
Copy link
Contributor Author

rmsrosa commented Nov 13, 2020

Many thanks for implementing this!

And thanks for telling me about DimensionlessQuantity. I was looking for something like this to use as you mentioned, but somehow I missed it.

As for the type piracy, the warning I get is "An imported function has been extended without using module defined typed arguments.", which shows up in julia-vscode/StaticLint.jl as "TypePiracy".

I just learned about this so I will just point to two places that explain it better: julialang docs: Avoid type piracy and discourse: Clarification on type piracy.

So I agree that my extension to dimtype is a type piracy since both types I use are defined in other modules, but the following extensions use the type DensityConcentration, which is defined in my module, but they still give me this warning:

edconvert(d::dimtype(Unitful.Density), x::Unitful.DimensionlessQuantity, e::DensityConcentration) = x * 1u"kg/l"

edconvert(d::dimtype(NoDims), x::Unitful.Density, e::DensityConcentration) = x * 1u"L/kg"

Anyways, I will try out now with the direct relation that you implemented and get back here to let you know.

@rmsrosa
Copy link
Contributor Author

rmsrosa commented Nov 13, 2020

Ah, I guess I need to wait till the pull-request is closed, no worries. And I like that you proposed to make the definition of DimensionlessQuantity in Unitful.jl consistent with the others.

@sostock
Copy link
Owner

sostock commented Nov 13, 2020

Thanks, I know about type piracy but did not know that there are linters who warn about it.

However, the edconvert methods are not type piracy (since you own DensityConcentration), so I wonder why you get a warning for them. Maybe the linter doesn’t recognise that DensityConcentration is a type from your module, since its definition is obfuscated by the @equivalence macro? Is the warning fixed by defining the DensityConcentration struct yourself instead of using the macro?

@rmsrosa
Copy link
Contributor Author

rmsrosa commented Nov 13, 2020

Is the warning fixed by defining the DensityConcentration struct yourself instead of using the macro?

If I define it directly then the warning disappears.

@sostock
Copy link
Owner

sostock commented Nov 13, 2020

If I define it directly then the warning disappears.

One more reason to remove the @equivalence macro 😄

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