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

Implement inverse trigonometric functions from ratio to angle. #184

Merged
merged 1 commit into from
May 7, 2020
Merged

Implement inverse trigonometric functions from ratio to angle. #184

merged 1 commit into from
May 7, 2020

Conversation

adamreichold
Copy link
Contributor

@adamreichold adamreichold commented May 5, 2020

Fixes #168

@adamreichold
Copy link
Contributor Author

@iliekturtles I am somewhat at a loss as to the correct usage of the Convert trait, i.e. I get

error[E0271]: type mismatch resolving `<V as Conversion<V>>::T == <si::angle::radian as Conversion<V>>::T`
  --> src/si/ratio.rs:76:9
   |
37 | impl<U, V> Ratio<U, V>
   |         - this type parameter
...
76 |         Angle::new::<radian>(self.value.atanh())
   |         ^^^^^^^^^^^^^^^^^^^^ expected type parameter `V`, found struct `si::angle::radian`
   |
   = note: expected associated type `<V as Conversion<V>>::T`
              found associated type `<si::angle::radian as Conversion<V>>::T`

Could you give me a hint?

Copy link
Owner

@iliekturtles iliekturtles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Sorry for my delay in responding, it has been a busy couple days. Let me know if you get any other compile errors once the fix I noted in the diff is applied.

src/si/ratio.rs Outdated Show resolved Hide resolved
@adamreichold
Copy link
Contributor Author

adamreichold commented May 5, 2020

@iliekturtles I wonder how to handle atan2. Personally, I only need it for Length values (to switch between coordinate systems), but strictly speaking it should apply to all pairs of quantities. Should I propose an implementation on si::Length or on the general Quantity?

@adamreichold adamreichold marked this pull request as ready for review May 5, 2020 19:45
@iliekturtles
Copy link
Owner

I think I'm inclined towards implementing for all quantities but I'm second guessing myself as I write this sentence. Do you have any examples where the function would be used on non-length quantities?

@adamreichold
Copy link
Contributor Author

adamreichold commented May 5, 2020

Do you have any examples where the function would be used on non-length quantities?

After some digging, I found a paper mentioning atan2 when computing magnetic field strengths (of rotationally symmetric sources) where the computation a field quantity expansion involves atan2.

I guess this is the basic example, any field which has a spherical or cylindrical symmetry might have modes which imply usage of atan2 but it is admittedly a bit fringe...

@Aehmlo
Copy link
Contributor

Aehmlo commented May 5, 2020

An example of using the arctangent non-length quantities which immediately comes to mind for me is the phase delay in viscoelastic solids, which is something I could see myself wanting to work with in uom in the future. Additionally, in physics (particularly particle physics), there's often a desire to work in momentum space/other spaces rather than position space.

@adamreichold
Copy link
Contributor Author

I think I'm inclined towards implementing for all quantities

An example of using the arctangent non-length quantities which immediately comes to mind

Amended the generic implementation on all si::Quantity values returning an si::Angle.

@iliekturtles Should be ready for review now.

@iliekturtles
Copy link
Owner

Changes look great! If you can amend the commit with tests similar to what is done for hypot in length.rs I'll merge!

uom/src/si/length.rs

Lines 85 to 102 in 4fceafc

#[cfg(all(test, feature = "std"))]
mod tests {
storage_types! {
types: Float;
use si::quantities::*;
use si::length::meter;
use tests::Test;
quickcheck! {
#[allow(trivial_casts)]
fn hypot(l: V, r: V) -> bool {
Test::eq(&l.hypot(r),
&Length::new::<meter>(l).hypot(Length::new::<meter>(r)).get::<meter>())
}
}
}
}

@adamreichold
Copy link
Contributor Author

If you can amend the commit with tests

Added property-based tests for all new functions.

@iliekturtles
Copy link
Owner

Clippy doesn't like test_nan_or_eq's parameters. Since the function is only going to be called for floats the references can be dropped.

diff --git a/src/si/ratio.rs b/src/si/ratio.rs
index 8ceee33..4a3f853 100644
--- a/src/si/ratio.rs
+++ b/src/si/ratio.rs
@@ -161,33 +161,33 @@ mod tests {
             use si::quantities::*;
             use tests::Test;

-            fn test_nan_or_eq(yl: &V, yr: &V) -> bool {
-                (yl.is_nan() && yr.is_nan()) || Test::eq(yl, yr)
+            fn test_nan_or_eq(yl: V, yr: V) -> bool {
+                (yl.is_nan() && yr.is_nan()) || Test::eq(&yl, &yr)
             }

             quickcheck! {
                 fn acos(x: V) -> bool {
-                    test_nan_or_eq(&x.acos(), &Ratio::from(x).acos().get::<a::radian>())
+                    test_nan_or_eq(x.acos(), Ratio::from(x).acos().get::<a::radian>())
                 }

                 fn acosh(x: V) -> bool {
-                    test_nan_or_eq(&x.acosh(), &Ratio::from(x).acosh().get::<a::radian>())
+                    test_nan_or_eq(x.acosh(), Ratio::from(x).acosh().get::<a::radian>())
                 }

                 fn asin(x: V) -> bool {
-                    test_nan_or_eq(&x.asin(), &Ratio::from(x).asin().get::<a::radian>())
+                    test_nan_or_eq(x.asin(), Ratio::from(x).asin().get::<a::radian>())
                 }

                 fn asinh(x: V) -> bool {
-                    test_nan_or_eq(&x.asinh(), &Ratio::from(x).asinh().get::<a::radian>())
+                    test_nan_or_eq(x.asinh(), Ratio::from(x).asinh().get::<a::radian>())
                 }

                 fn atan(x: V) -> bool {
-                    test_nan_or_eq(&x.atan(), &Ratio::from(x).atan().get::<a::radian>())
+                    test_nan_or_eq(x.atan(), Ratio::from(x).atan().get::<a::radian>())
                 }

                 fn atanh(x: V) -> bool {
-                    test_nan_or_eq(&x.atanh(), &Ratio::from(x).atanh().get::<a::radian>())
+                    test_nan_or_eq(x.atanh(), Ratio::from(x).atanh().get::<a::radian>())
                 }
             }
         }

@iliekturtles iliekturtles merged commit bdb7372 into iliekturtles:master May 7, 2020
@adamreichold adamreichold deleted the inverse-trigonometric-functions branch May 7, 2020 16:06
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 this pull request may close these issues.

Feature requrest: Need asin, acos, atan{,2} support on length
3 participants