Support for albedo parameters in Spacecraft state#545
Support for albedo parameters in Spacecraft state#545ChristopherRabotin wants to merge 1 commit into
Conversation
- Added AlbedoCr to StateParameter enum. - Updated Spacecraft struct to include albedo: AlbedoData. - Increased Spacecraft state size to 10 (Position, Velocity, Cr, Cd, Mass, Albedo Cr) and its propagation vector length to 110 (to include 10x10 STM). - Updated State trait implementation for Spacecraft to handle the new dimension. - Updated SpacecraftDynamics to support the increased state size. - Updated MvnSpacecraft, KfEstimate, SpacecraftUncertainty, and Parquet import logic to handle the new state dimension and Albedo Cr parameter. This lays the foundation for implementing the Albedo radiation pressure force model. Co-authored-by: ChristopherRabotin <4823784+ChristopherRabotin@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request integrates albedo configuration into the spacecraft model, increasing the state vector size from 9 to 10. Key changes include the addition of AlbedoData to the Spacecraft struct, updates to the State Transition Matrix (STM) dimensions, and adjustments to dynamics, Monte Carlo dispersions, and orbital determination logic to support the new AlbedoCr parameter. Review feedback focused on enhancing code maintainability by replacing hardcoded constants with associated types from the State trait and recommended updating several doc comments to correctly describe the expanded state vector layout.
| pub thrust_direction: Option<ThrustDirection>, | ||
| /// Optionally stores the state transition matrix from the start of the propagation until the current time (i.e. trajectory STM, not step-size STM) | ||
| /// STM is contains position and velocity, Cr, Cd, prop mass | ||
| /// STM is contains position and velocity, Cr, Cd, prop mass, Albedo Cr |
There was a problem hiding this comment.
The comment contains a typo ("is contains") and should ideally use a conjunction for the last item in the list.
| /// STM is contains position and velocity, Cr, Cd, prop mass, Albedo Cr | |
| /// STM contains position and velocity, Cr, Cd, prop mass, and Albedo Cr |
| fn to_vector(&self) -> OVector<f64, Const<110>> { | ||
| let mut vector = OVector::<f64, Const<110>>::zeros(); |
There was a problem hiding this comment.
Consider using the associated type Self::VecLength instead of hardcoding Const<110> for better maintainability and consistency with the trait definition.
| fn to_vector(&self) -> OVector<f64, Const<110>> { | |
| let mut vector = OVector::<f64, Const<110>>::zeros(); | |
| fn to_vector(&self) -> OVector<f64, Self::VecLength> { | |
| let mut vector = OVector::<f64, Self::VecLength>::zeros(); |
| /// [X, Y, Z, Vx, Vy, Vz, Cr, Cd, Fuel mass, STM(9x9)] | ||
| fn set(&mut self, epoch: Epoch, vector: &OVector<f64, Const<90>>) { | ||
| /// [X, Y, Z, Vx, Vy, Vz, Cr, Cd, Fuel mass, Albedo Cr, STM(10x10)] | ||
| fn set(&mut self, epoch: Epoch, vector: &OVector<f64, Const<110>>) { |
There was a problem hiding this comment.
Consider using the associated type Self::VecLength instead of hardcoding Const<110>.
| fn set(&mut self, epoch: Epoch, vector: &OVector<f64, Const<110>>) { | |
| fn set(&mut self, epoch: Epoch, vector: &OVector<f64, Self::VecLength>) { |
| self.albedo.coeff_reflectivity = sc_state[9].clamp(0.0, 2.0); | ||
| } | ||
|
|
||
| /// diag(STM) = [X,Y,Z,Vx,Vy,Vz,Cr,Cd,Fuel] |
There was a problem hiding this comment.
The comment describing the STM diagonal layout is outdated and should be updated to include the newly added AlbedoCr parameter.
| /// diag(STM) = [X,Y,Z,Vx,Vy,Vz,Cr,Cd,Fuel] | |
| /// diag(STM) = [X,Y,Z,Vx,Vy,Vz,Cr,Cd,Fuel,AlbedoCr] |
| ) -> Result<(OVector<f64, Const<9>>, OMatrix<f64, Const<9>, Const<9>>), DynamicsError> { | ||
| ) -> Result<(OVector<f64, Const<10>>, OMatrix<f64, Const<10>, Const<10>>), DynamicsError> { | ||
| // Rebuild the appropriately sized state and STM. | ||
| // This is the orbital state followed by Cr and Cd |
There was a problem hiding this comment.
This comment is now incomplete as the state vector has been expanded to include PropMass and Albedo Cr.
| // This is the orbital state followed by Cr and Cd | |
| // This is the orbital state followed by Cr, Cd, PropMass, and Albedo Cr |
| state_deviation: OVector::<f64, Const<10>>::zeros(), // Deviation not stored | ||
| covar, | ||
| covar_bar: covar, // Not stored, use covar | ||
| stm: OMatrix::<f64, Const<9>, Const<9>>::identity(), // Not stored | ||
| stm: OMatrix::<f64, Const<10>, Const<10>>::identity(), // Not stored |
There was a problem hiding this comment.
It is better to use the associated types from the State trait (e.g., <Spacecraft as State>::Size) instead of hardcoding Const<10> to ensure consistency and easier maintenance if the state size changes again in the future. Additionally, ensure these fields are necessary for the struct, as Rule 2 advises removing unused fields from update logic.
| state_deviation: OVector::<f64, Const<10>>::zeros(), // Deviation not stored | |
| covar, | |
| covar_bar: covar, // Not stored, use covar | |
| stm: OMatrix::<f64, Const<9>, Const<9>>::identity(), // Not stored | |
| stm: OMatrix::<f64, Const<10>, Const<10>>::identity(), // Not stored | |
| state_deviation: OVector::<f64, <Spacecraft as State>::Size>::zeros(), // Deviation not stored | |
| covar, | |
| covar_bar: covar, // Not stored, use covar | |
| stm: OMatrix::<f64, <Spacecraft as State>::Size, <Spacecraft as State>::Size>::identity(), // Not stored |
References
- Avoid updating properties that are not used by the consuming struct. Unused fields should be removed from update logic to avoid unnecessary complexity.
1 participant
I have enhanced the core
Spacecraftstate and related structures to support the upcoming albedo radiation pressure modeling.Key changes:
AlbedoCrtoStateParameterinnyx-core/src/md/param.rs.albedo: AlbedoData(aliased fromSRPData) to theSpacecraftstruct innyx-core/src/cosmic/spacecraft.rs.Statetrait forSpacecraftto correctly map the new 10th parameter (Albedo Cr) and its partials in the STM.SpacecraftDynamicsinnyx-core/src/dynamics/spacecraft.rsto reflect the new state dimensions.MvnSpacecraft(Monte Carlo),KfEstimate(Kalman Filter),SpacecraftUncertainty, and the solution import logic to accommodate the 10-dimensional state.These changes ensure that Albedo reflectivity can be tracked, propagated, and potentially estimated in the future, providing the necessary infrastructure for the Albedo force model implementation.
Fixes #316
PR created automatically by Jules for task 4147399694482142674 started by @ChristopherRabotin