uom icon indicating copy to clipboard operation
uom copied to clipboard

Add photometric units

Open quentinmit opened this issue 2 years ago • 3 comments

This PR adds LuminousFlux and Illuminance units. Unfortunately they are dimensionally equal to LuminousIntensity and Luminance. I tried to provide a Mul trait implementation to construct LuminousFlux from LuminousIntensity * SolidAngle, but try as I might I couldn't get the compiler to allow me to provide the trait. I saw #189 and related issues, but I don't think specialization is necessarily the solution - the compiler knows that LuminousIntensity * SolidAngle isn't allowed, so it shouldn't be a duplicate implementation... I dunno. Any advice is greatly appreciated.

quentinmit avatar Jul 07 '22 07:07 quentinmit

Thanks for the PR! I'll review in detail over the next few days.

In regards to the Mul impl the problem is that the Kind associated type defaults to uom::Kind. I'm not sure/don't remember why the default isn't to maintain one of the operand's kinds -- if this even makes sense. I'll do some more digging into this as well but I'm not hopeful of a good short-term solution beyond .into().

type Output = ... doesn't specify a kind which means the default is used. https://github.com/iliekturtles/uom/blob/51a7cbf8238b0faa98b8f882614747d21cfb2f68/src/system.rs#L469-L484

iliekturtles avatar Jul 07 '22 13:07 iliekturtles

In regards to the Mul impl the problem is that the Kind associated type defaults to uom::Kind. I'm not sure/don't remember why the default isn't to maintain one of the operand's kinds -- if this even makes sense. I'll do some more digging into this as well but I'm not hopeful of a good short-term solution beyond .into().

type Output = ... doesn't specify a kind which means the default is used.

Naively, I would expect that by default the operators should only be provided for the case of A<$kind> + B<$kind> = Output<$kind>, so the example of LuminousIntensity * SolidAngle should be treated as Kind * SolidAngleKind and not have a Mul impl at all unless it's provided by a separate impl. By the way, the compiler error I get is:

error[E0308]: mismatched types
   --> src/si/luminous_flux.rs:109:26
    |
109 | /                          &(LuminousIntensity::<V>::new::<li::candela>((*l).clone())
110 | |                            * SolidAngle::<V>::new::<sa::steradian>((*r).clone())))
    | |_________________________________________________________________________________^ expected trait `LuminousFluxKind`, found trait `Kind`

so the compiler knows it's looking for a LuminousFluxKind. I was hoping that would make it select the alternate Mul implementation that can produce a LuminousFluxKind.

quentinmit avatar Jul 07 '22 17:07 quentinmit

Only implementing Mul and Div for Kind = uom::Kind would require way too much work to implement a custom kind (10 Mul and Div implementations multiplied by all of the by-val by-ref combinations!) A simple implementation with generics Kl and Kr (LHS and RHS's kind) would require specialization.

What might be a possible path to pursue would be to implement Mul and Div for all Kind * Kind combinations. This would require the number of kinds squared impls (72 right now). With a macro this probably wouldn't be too painful:

impl_kind_ops!(Kind * Kind = Kind);
impl_kind_ops!(LuminousIntensityKind * Kind = Kind);
impl_kind_ops!(LuminousIntensityKind * SolidAngleKind = LuminousFluxKind);
// ...

What might be an issue is the Mul/Div impls for Quantity. I tried to do something similar for Dimension and I can't remember if things didn't work or if there was just an even larger explosion of type bounds that currently exists. The current mul implementation would be changed to use $MulDivAlias (a shortcut for <A as Mul<V>>::Output to determine what the output Kind should be instead of defaulting to uom::Kind.

impl<Dl, Kl, Dr, Kr, U, V> Mul<Quantity<Dr, U, V>> for Quantity<Dl, U, V>
where
    Dl: Dimension<Kind = Kl>,
    Dr: Dimension<Kind = Kr>,
    // ...
{
    type Output = Quantity<
        $quantities<$($crate::typenum::$AddSubAlias<Dl::$symbol, Dr::$symbol>,)+ Kind = $MulDivAlias<Kl, Kr>>,
         U, V>;

    fn mul(/*...*/) -> Self::Output {
        // ...
    }
}

Because of the scope of this work I'd like to see this effort separated from this PR. We can get Illuminance and LuminousFlux merged using the current functionality while finding out a better way to work with Kinds.

Possibly use #315 as the tracking issue and re-state the original problem.

iliekturtles avatar Jul 19 '22 00:07 iliekturtles