Add photometric units by quentinmit · Pull Request #313 · iliekturtles/uom

quentinmit · 2022-07-07T07:37:24Z

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.

iliekturtles · 2022-07-07T13:25:03Z

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.

uom/src/system.rs

Lines 469 to 484 in 51a7cbf

    
           impl<Dl, Dr, Ul, Ur, V> $crate::lib::ops::$MulDivTrait<Quantity<Dr, Ur, V>> 
        
               for Quantity<Dl, Ul, V> 
        
           where 
        
               Dl: Dimension + ?Sized, 
        
               $(Dl::$symbol: $crate::lib::ops::$AddSubTrait<Dr::$symbol>, 
        
               <Dl::$symbol as $crate::lib::ops::$AddSubTrait<Dr::$symbol>>::Output: $crate::typenum::Integer,)+ 
        
               Dl::Kind: $crate::marker::$MulDivTrait, 
        
               Dr: Dimension + ?Sized, 
        
               Dr::Kind: $crate::marker::$MulDivTrait, 
        
               Ul: Units<V> + ?Sized, 
        
               Ur: Units<V> + ?Sized, 
        
               V: $crate::num::Num + $crate::Conversion<V> + $crate::lib::ops::$MulDivTrait<V>, 
        
           { 
        
               type Output = Quantity< 
        
                   $quantities<$($crate::typenum::$AddSubAlias<Dl::$symbol, Dr::$symbol>,)+>, 
        
                   Ul, V>;

quentinmit · 2022-07-07T17:02:18Z

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.

iliekturtles

Finally got though a first pass at reviewing. I still haven't reviewed the mul impl in detail yet, but that is next on my list.

iliekturtles · 2022-07-12T18:21:48Z

+//! Illuminance (base unit lux, lx, cd · sr / m²).
+
+quantity! {
+    /// Illuminance (base unit lux, lx, cd · sr / m²).
+    quantity: Illuminance; "illuminance";
+    /// Dimension of illuminance, E (base unit lux, lx, cd · sr / m²).


Suggested change

//! Illuminance (base unit lux, lx, cd · sr / m²).

quantity! {

/// Illuminance (base unit lux, lx, cd · sr / m²).

quantity: Illuminance; "illuminance";

/// Dimension of illuminance, E (base unit lux, lx, cd · sr / m²).

//! Illuminance (base unit lux, m⁻² · cd).

quantity! {

/// Illuminance (base unit lux, m⁻² · cd).

quantity: Illuminance; "illuminance";

/// Dimension of illuminance, L⁻²J (base unit lux, m⁻² · cd).

By convention just the base 7 units are included. I've excluded the steradian (sr) because it's not a base unit, but I'm not convinced this is the right choice.

For the dimension comment the base unit dimensions are used. There isn't a current place where a quantity's symbol is defined or used anywhere. I'll add an issue about this.

iliekturtles · 2022-07-12T18:23:47Z

+        /// The lux is defined to be 1 lumen per square meter,
+        /// or 1 candela steradian per square meter.


Suggested change

/// The lux is defined to be 1 lumen per square meter,

/// or 1 candela steradian per square meter.

/// Dervied unit of illuminance. The lux is defined to be 1 lumen per square meter, or 1

/// candela steradian per square meter.

iliekturtles · 2022-07-12T18:23:51Z

+        @decalux: prefix!(deca); "dalm", "decalux", "decaluxs";
+        /// The lux is defined to be 1 lumen per square meter,
+        /// or 1 candela steradian per square meter.
+        @lux: prefix!(none); "lm", "lux", "luxs";


Suggested change

@lux: prefix!(none); "lm", "lux", "luxs";

@lux: prefix!(none); "lx", "lux", "lux";

Copy/paste error from luminous flux? It also seems the plural and singular are the same.

iliekturtles · 2022-07-12T18:23:57Z

+        quickcheck! {
+            #[allow(trivial_casts)]
+            fn add(l: A<V>, r: A<V>) -> bool {
+                Test::eq(&Illuminance::<V>::new::<i::lux>(&*l / &*r),
+                         &(LuminousFlux::<V>::new::<lf::lumen>((*l).clone())
+                           / Area::<V>::new::<a::square_meter>((*r).clone())).into())
+            }
+        }


Rather than doing quickcheck test, will you replace with a test to confirm the dimensions as well as another test to confirm the derived units. See other quantities such as MagneticFlux for an example:

uom/src/si/magnetic_flux.rs

Lines 53 to 87 in 51a7cbf

#[test]

fn check_dimension() {

let _: MagneticFlux<V> = ElectricPotential::new::<v::volt>(V::one())

* Time::new::<t::second>(V::one());

}

#[test]

fn check_units() {

test::<v::yottavolt, f::yottaweber>();

test::<v::zettavolt, f::zettaweber>();

test::<v::exavolt, f::exaweber>();

test::<v::petavolt, f::petaweber>();

test::<v::teravolt, f::teraweber>();

test::<v::gigavolt, f::gigaweber>();

test::<v::megavolt, f::megaweber>();

test::<v::kilovolt, f::kiloweber>();

test::<v::hectovolt, f::hectoweber>();

test::<v::decavolt, f::decaweber>();

test::<v::volt, f::weber>();

test::<v::decivolt, f::deciweber>();

test::<v::centivolt, f::centiweber>();

test::<v::millivolt, f::milliweber>();

test::<v::microvolt, f::microweber>();

test::<v::nanovolt, f::nanoweber>();

test::<v::picovolt, f::picoweber>();

test::<v::femtovolt, f::femtoweber>();

test::<v::attovolt, f::attoweber>();

test::<v::zeptovolt, f::zeptoweber>();

test::<v::yoctovolt, f::yoctoweber>();

fn test<U: v::Conversion<V>, F: f::Conversion<V>>() {

Test::assert_approx_eq(&MagneticFlux::new::<F>(V::one()),

&(ElectricPotential::new::<U>(V::one())

* Time::new::<t::second>(V::one())));

}

iliekturtles · 2022-07-12T18:34:24Z

+        @kilolumen: prefix!(kilo); "klm", "kilolumen", "kilolumens";
+        @hectolumen: prefix!(hecto); "hlm", "hectolumen", "hectolumens";
+        @decalumen: prefix!(deca); "dalm", "decalumen", "decalumens";
+        /// The lumen is defined to be 1 candela steradian.


Suggested change

/// The lumen is defined to be 1 candela steradian.

/// Derived unit of luminous flux. The lumen is defined to be 1 candela steradian.

iliekturtles · 2022-07-19T00:42:31Z

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 (7² 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.

quentinmit added 2 commits July 7, 2022 03:18

Add LuminousFlux

e325349

Add Illuminance

746b5fa

iliekturtles reviewed Jul 12, 2022

View reviewed changes

iliekturtles mentioned this pull request Jul 19, 2022

Multiply Ratio and Angle #315

Open

iliekturtles mentioned this pull request Aug 11, 2022

Add TemperatureGradient quantity with related units. #349

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add photometric units#313

Add photometric units#313
quentinmit wants to merge 2 commits into
iliekturtles:masterfrom
quentinmit:photometric

quentinmit commented Jul 7, 2022

Uh oh!

iliekturtles commented Jul 7, 2022

Uh oh!

quentinmit commented Jul 7, 2022

Uh oh!

iliekturtles left a comment

Uh oh!

iliekturtles Jul 12, 2022

Uh oh!

iliekturtles Jul 12, 2022

Uh oh!

iliekturtles Jul 12, 2022

Uh oh!

iliekturtles Jul 12, 2022

Uh oh!

iliekturtles Jul 12, 2022

Uh oh!

iliekturtles commented Jul 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		/// The lux is defined to be 1 lumen per square meter,
		/// or 1 candela steradian per square meter.

	@lux: prefix!(none); "lm", "lux", "luxs";
	@lux: prefix!(none); "lx", "lux", "lux";

	#[test]
	fn check_dimension() {
	let _: MagneticFlux<V> = ElectricPotential::new::<v::volt>(V::one())
	* Time::new::<t::second>(V::one());
	}

	#[test]
	fn check_units() {
	test::<v::yottavolt, f::yottaweber>();
	test::<v::zettavolt, f::zettaweber>();
	test::<v::exavolt, f::exaweber>();
	test::<v::petavolt, f::petaweber>();
	test::<v::teravolt, f::teraweber>();
	test::<v::gigavolt, f::gigaweber>();
	test::<v::megavolt, f::megaweber>();
	test::<v::kilovolt, f::kiloweber>();
	test::<v::hectovolt, f::hectoweber>();
	test::<v::decavolt, f::decaweber>();
	test::<v::volt, f::weber>();
	test::<v::decivolt, f::deciweber>();
	test::<v::centivolt, f::centiweber>();
	test::<v::millivolt, f::milliweber>();
	test::<v::microvolt, f::microweber>();
	test::<v::nanovolt, f::nanoweber>();
	test::<v::picovolt, f::picoweber>();
	test::<v::femtovolt, f::femtoweber>();
	test::<v::attovolt, f::attoweber>();
	test::<v::zeptovolt, f::zeptoweber>();
	test::<v::yoctovolt, f::yoctoweber>();

	fn test<U: v::Conversion<V>, F: f::Conversion<V>>() {
	Test::assert_approx_eq(&MagneticFlux::new::<F>(V::one()),
	&(ElectricPotential::new::<U>(V::one())
	* Time::new::<t::second>(V::one())));
	}

	/// The lumen is defined to be 1 candela steradian.
	/// Derived unit of luminous flux. The lumen is defined to be 1 candela steradian.

Conversation

quentinmit commented Jul 7, 2022

Uh oh!

iliekturtles commented Jul 7, 2022

Uh oh!

quentinmit commented Jul 7, 2022

Uh oh!

iliekturtles left a comment

Choose a reason for hiding this comment

Uh oh!

iliekturtles Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

iliekturtles Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

iliekturtles Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

iliekturtles Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

iliekturtles Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

iliekturtles commented Jul 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants