-
Notifications
You must be signed in to change notification settings - Fork 97
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
Add photometric units #313
base: master
Are you sure you want to change the base?
Conversation
Thanks for the PR! I'll review in detail over the next few days. In regards to the
Lines 469 to 484 in 51a7cbf
|
Naively, I would expect that by default the operators should only be provided for the case of
so the compiler knows it's looking for a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
//! 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²). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//! 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.
/// The lux is defined to be 1 lumen per square meter, | ||
/// or 1 candela steradian per square meter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// 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. |
@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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
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()) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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()))); | |
} |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// The lumen is defined to be 1 candela steradian. | |
/// Derived unit of luminous flux. The lumen is defined to be 1 candela steradian. |
Only implementing What might be a possible path to pursue would be to implement 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 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 Possibly use #315 as the tracking issue and re-state the original problem. |
This PR adds
LuminousFlux
andIlluminance
units. Unfortunately they are dimensionally equal toLuminousIntensity
andLuminance
. I tried to provide aMul
trait implementation to constructLuminousFlux
fromLuminousIntensity * 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 thatLuminousIntensity * SolidAngle
isn't allowed, so it shouldn't be a duplicate implementation... I dunno. Any advice is greatly appreciated.