-
Notifications
You must be signed in to change notification settings - Fork 22
Update method used to find mean spin-axis for Pointing to just averag… #2533
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
Update method used to find mean spin-axis for Pointing to just averag… #2533
Conversation
…e instantaneous z-axes across Pointing
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.
Pull request overview
This PR refactors the method for computing the mean spin-axis for the Pointing frame, replacing quaternion averaging with a simpler approach that directly averages the instantaneous z-axes expressed in HAE Cartesian coordinates.
Key Changes
- Replaced
_average_quaternions()with_mean_spin_axis()which usesframe_transform()to get instantaneous z-axes and averages them - Simplified
_create_rotation_matrix()to accept the mean z-axis directly instead of computing it from an averaged quaternion - Updated tests to reflect the new function names and expected values
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| imap_processing/spice/pointing_frame.py | Replaced quaternion averaging algorithm with direct z-axis averaging; added frame_transform import; updated function signatures and documentation |
| imap_processing/tests/spice/test_pointing_frame.py | Updated test function names and expected values to match the new implementation; tests now validate z-axis output instead of quaternion output |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| y_avg = np.cross(z_avg, [0, 0, 1]) | ||
| # x_avg is perpendicular to y_avg and z_avg. | ||
| x_avg = np.cross(y_avg, z_avg) |
Copilot
AI
Dec 22, 2025
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 y_avg and x_avg vectors should be normalized to ensure the rotation matrix is orthonormal. Currently, the cross products produce vectors whose magnitudes depend on the input vectors. Without normalization, the rotation matrix may not preserve vector lengths during transformations, which could lead to incorrect results.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
greglucas
left a comment
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.
Nice, it actually looks like a good simplification here too.
I added a few nitpicky comments, but the overall logic looks good to me!
| z_inertial_hae = frame_transform( | ||
| et_times, np.array([0, 0, 1]), SpiceFrame.IMAP_SPACECRAFT, SpiceFrame.ECLIPJ2000 | ||
| ) | ||
|
|
||
| # Compute the average spin axis by averaging each component across time | ||
| z_avg = np.mean(z_inertial_hae, axis=0) |
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.
A couple of potential thoughts/questions here.
What is et_times being requested as? Would it make sense to sample this at a higher frequency under the hood here, to try and get closer to the actual quaternions that were input.
Should we compute the standard deviation and log/warn if it is outside of some bound for awareness purposes?
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.
As it is now, we are sampling ~10 times per spin. IMO, ideally, we should be pulling the quaternions straight from the attitude history files which I think are 1Hz and would give ~15/spin.
I could pull spin 70 data and add a test that checks that we get close to what GLOWS gets.
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.
IMO, ideally, we should be pulling the quaternions straight from the attitude history files which I think are 1Hz and would give ~15/spin
If you sample the data at 1Hz do we exactly get back the instantaneous value based on the quaternion, or is it somehow smoothed even on the exact data points so that we have a smooth curve but not match the "true" data? i.e. I'm wondering if we can just adjust our sampling period here to be what the quaternions are included at rather than this slight offset to avoid any interpolation/sampling.
90f72e4
into
IMAP-Science-Operations-Center:dev
…e instantaneous z-axes across Pointing
Change Summary
Overview
Instead of averaging quaternions, just average the instantaneous z-axis expressed in HAE Cartesian coordinates.
Updated Files
_average_quaternionsto_mean_spin_axisand update functionality to do as the name change implies._create_rotation_matrixcan now just use the mean z-axis as computed.Closes: #2532