Rewrite coords module in pure Rust #126
Conversation
jbangelo
force-pushed
the
jbangelo/riir-coords
branch
3 times, most recently
from
3e3db5b to
fd6d2ca
Compare
jbangelo
force-pushed
the
jbangelo/riir-coords
branch
from
34f83a3 to
64293ba
Compare
| #[must_use] | ||
| pub fn new( | ||
| reference_frame: ReferenceFrame, | ||
| position: ECEF, | ||
| velocity: Option<ECEF>, | ||
| epoch: GpsTime, | ||
| ) -> Self { | ||
| Coordinate { | ||
| reference_frame, | ||
| position, | ||
| velocity, | ||
| epoch, | ||
| } | ||
| } | ||
|
|
||
| /// Create a new [`Coordinate`] object with a velocity value | ||
| #[must_use] | ||
| pub fn without_velocity( | ||
| reference_frame: ReferenceFrame, | ||
| position: ECEF, | ||
| epoch: GpsTime, | ||
| ) -> Self { | ||
| Coordinate { | ||
| reference_frame, | ||
| position, | ||
| velocity: None, | ||
| epoch, | ||
| } | ||
| } | ||
|
|
||
| /// Create a new [`Coordinate`] object with no velocity | ||
| #[must_use] | ||
| pub fn with_velocity( | ||
| reference_frame: ReferenceFrame, | ||
| position: ECEF, | ||
| velocity: ECEF, | ||
| epoch: GpsTime, | ||
| ) -> Self { | ||
| Coordinate { | ||
| reference_frame, | ||
| position, | ||
| velocity: Some(velocity), | ||
| epoch, | ||
| } | ||
| } |
There was a problem hiding this comment.
This might be a good case to use a crate like bon, typed-builder or something similar to make a builder. You could do something like this
use bon::Builder
/// Complete coordinate used for transforming between reference frames
///
/// Velocities are optional, but when present they will be transformed
#[derive(Debug, PartialEq, PartialOrd, Clone, Copy, Builder)]
pub struct Coordinate {
reference_frame: ReferenceFrame,
position: ECEF,
velocity: Option<ECEF>,
epoch: GpsTime,
}
// ...
Coordinate::builder()
.reference_frame(ref_frame)
.position(pos)
.velocity(vel) // could also use .maybe_velocity() to pass in Option<ECEF>
.epoch(epoch)
.build();It uses compile time checks to ensure you don't miss any fields either but it might make having to create all sorts of different new combinations a bit easier to manage.
There was a problem hiding this comment.
That's a good point. I'll save it for a follow up PR.
jbangelo
force-pushed
the
jbangelo/riir-coords
branch
from
bef2294 to
4f18010
Compare
JADC362
left a comment
JADC362
left a comment
There was a problem hiding this comment.
LGTM! I left minor nits and comments, but overall looks clear, and all the From conversions are really handy.
swiftnav/src/coords/ecef.rs
Outdated
| // not divide by zero as only one of S or C should ever be zero. | ||
| // | ||
| // This incurs an extra division each iteration which the author was | ||
| // explicityl trying to avoid and it may be that this solution is just |
There was a problem hiding this comment.
nit
| // explicityl trying to avoid and it may be that this solution is just | |
| // explicitly trying to avoid and it may be that this solution is just |
Also, would we like to leave a TODO or something here since you mention it may need more thought?
Perhaps I haven't understood the problem well, but what is the issue of having this extra division? Is it because it adds an extra step affecting performance or because it can lead to instability again?
If the latter is true, we could alternatively panic here if the number becomes "unstably large", I guess it may be better to panic than to return an incorrect number that we think is ok.
Again, perhaps all this answer is completely off, not sure if I understood the problem correctly.
There was a problem hiding this comment.
The comments here are a direct copy/paste from https://github.com/swift-nav/libswiftnav/blob/master/src/coord_system.c, so if we were to follow up on this it should be there first.
I think the decision to pick this particular algorithm was because it avoided transcendental functions in the inner loop and it limited the number of division operations (which tend to take longer than addition/multiplication) so the comment is saying the addition of the scaling step adds a new division operation in the inner loop which wasn't present in the original paper which might undo some of the performance gain the original paper claimed. I don't think it's a blocking issue, or really anything we would want to follow up on.
| } | ||
| } | ||
|
|
||
| /// Computes the square root of a given number at compile time using the Newton-Raphson method. |
There was a problem hiding this comment.
You just bring me memories from university with the "Newton-Raphson method" 😅
Co-authored-by: Alejandro Duarte <32496764+JADC362@users.noreply.github.com>
Yet another chunk of changes from #122. We're getting close to the end, only one or two more!
This one converts the coordinates module into Rust. There's a bit more than just a re-write in this one, I've cleaned up a few things and included an additional refactors to lay the groundwork for some potential additions in the future.
What's been changed
nalgebra::Vectortype under the hood.as_array_ref(), andas_mut_array_ref()on the coordinate types have been changed to only available in the crate (will remove them eventually once the C library dependency is removed)Defaultimplementations are now derived instead of manually implementedAzimuthElevationhas been changed to also use analgebra::Vector2under the hood instead having two named fields.What's been removed
as_ptr(), andas_mut_ptr()functions on the coordinate types have been removedAsRefon the coordinate types have been removedWhat's been added
impl From<LLHDegrees> for ECEF, not sure how that had been missed previouslyimpl From<LLHRadians> for ECEFEllipsoidtrait since it's the same for any ellipsoid once the ellipsoid is defined