Add a few more uses of nalgebra #132
Conversation
| assert!( | ||
| s >= 0.0 && s <= 1.0, | ||
| "Can only compute square root of numbers between 0 and 1" | ||
| ); |
There was a problem hiding this comment.
I was about to say we shouldn't do this but realized this is a const fn, nice job! This is a great use of that.
There was a problem hiding this comment.
I don't understand this comment, but I am an absolute Rust noob. I take it to mean we shouldn't assert! in the code (vs debug_assert! presumably), but since the function is const the assert is expected to happen at compile-time? can't a const fn also be called at runtime?
There was a problem hiding this comment.
@peddie You've got the right idea. Asserts are generally considered an anti-pattern in Rust, except for in test code where asserts are the primary means of implementing checks (a failed assert causes the test to fail). const functions are also a bit of a special case. When a const function is evaluated at compile time a failed assert will cause a compilation error (like the one below). You're also right that a const function can be called at runtime, and I don't think there's a Rust equivalent to C++'s consteval to prevent it from being invoked at runtime. My hope is that a big ugly name like compile_time_sqrt will make clear my intention to not use this at runtime but it's certainly not foolproof.
error[E0080]: evaluation panicked: Can only compute square root of numbers between 0 and 1
--> swiftnav/src/coords/ellipsoid.rs:22:16
|
22 | const X: f64 = compile_time_sqrt(2.0);
| ^^^^^^^^^^^^^^^^^^^^^^ evaluation of `coords::ellipsoid::X` failed inside this call
|
note: inside `compile_time_sqrt`
--> swiftnav/src/math.rs:40:5
|
40 | / assert!(
41 | | s >= 0.0 && s <= 1.0,
42 | | "Can only compute square root of numbers between 0 and 1"
43 | | );
| |_____^ the failure occurred here
For more information about this error, try `rustc --explain E0080`.
error: could not compile `swiftnav` (lib) due to 1 previous error
I think there's a couple of alternatives if we don't like this, but none are perfect:
- Could move it into the
Ellipsoidtrait where it's used to maybe make clear it's not intended for general purpose use (though I think this would make it accessible outside this crate) - Could get rid of the compile-time nature at all and simply re-compute the Ellipsoid flattening term as needed at run time
| pub fn transform_position(&self, position: &ECEF, epoch: f64) -> ECEF { | ||
| let dt = epoch - self.epoch; | ||
| let tx = (self.tx + self.tx_dot * dt) * Self::TRANSLATE_SCALE; | ||
| let ty = (self.ty + self.ty_dot * dt) * Self::TRANSLATE_SCALE; | ||
| let tz = (self.tz + self.tz_dot * dt) * Self::TRANSLATE_SCALE; | ||
| let t = (self.t + self.t_dot * dt) * Self::TRANSLATE_SCALE; | ||
| let s = (self.s + self.s_dot * dt) * Self::SCALE_SCALE; | ||
| let rx = (self.rx + self.rx_dot * dt) * Self::ROTATE_SCALE; | ||
| let ry = (self.ry + self.ry_dot * dt) * Self::ROTATE_SCALE; | ||
| let rz = (self.rz + self.rz_dot * dt) * Self::ROTATE_SCALE; | ||
| let r = (self.r + self.r_dot * dt) * Self::ROTATE_SCALE; | ||
|
|
||
| let x = position.x() + tx + (s * position.x()) + (-rz * position.y()) + (ry * position.z()); | ||
| let y = position.y() + ty + (rz * position.x()) + (s * position.y()) + (-rx * position.z()); | ||
| let z = position.z() + tz + (-ry * position.x()) + (rx * position.y()) + (s * position.z()); | ||
| let m = Self::make_rotation_matrix(s, r); |
There was a problem hiding this comment.
I think you added a few allocations to the heap here with after changing this to nalgebra function, and adding the rotation matrix. probably doesn't matter but just making sure.
There was a problem hiding this comment.
Are you sure? From the nalgebra docs:
Internally, dynamically- and statically-sized matrices do not use the same data storage type. While the former is always allocated on the heap using a Vec, the latter prefers static allocation indirectly using a column-major 2D array [[T; R]; C] for better performances.
My understanding is that fixed size matrices will not allocate memory. There's likely to still be a bit of overhead from operating on objects instead of primitive data types, and likely from the new function call (unless the compiler can inline it, I'm still unclear what/where to to use the #[inline] attribute).
jbangelo
force-pushed
the
jbangelo/more-nalgebra
branch
from
91344f0 to
0c2fb40
Compare
This PR updates the Reference frame module to use the
nalgebracrate for more of the math.