Refactor GaussMarkov sensor noise handling #881

Natsoulas · 2024-12-21T19:52:00Z

Tickets addressed: issue Inconsistent sensor noise handling #867
Review: By commit
Merge strategy: Merge (no squash)

Description

Followed user feedback about inconsistencies in how various sensors handle the GaussMarkov noise model. This PR addresses all the issues pointed out in issue #867 and updates relevant documentation. Necessary private class attributes are made accessible outside of the class via getter and setter methods introduced in this PR. Additionally, I edited sensor modules to initialize the GM noise models in their constructors rather than in their Reset() methods (this was only some sensors).

Verification

Each change/refactor was verified via passing unit tests and running scenarioGaussMarkovRandomWalk.py to ensure the refactor did not change any functionality of the code.

Documentation

Changed imuSensor name for walkBounds to errorBounds, and documented latex doc for the IMU Sensor called imuSensor/_Documentation/BasiliskCorruptions.tex

Future work

See if community desires any additional refactoring or clarification of Gauss Markov sensor noise modeling through additional documentation

sdunlap-afit · 2024-12-23T13:20:22Z

@Natsoulas Looking through the code, it seems ImuSensor::Reset() is still setting the two AMatrix variables to I. If you remove these from Reset(), you may also want to remove this comment in scenarioGaussMarkovRandomWalk.py to avoid confusion: # Set IMU2's A Matrix to zero AFTER initialization.

Natsoulas · 2024-12-23T17:54:58Z

Thanks @sdunlap-afit for looking over my changes. I'll incorporate that change this afternoon.

… constructor

…reset method

Natsoulas · 2024-12-23T23:06:25Z

@sdunlap-afit I just removed the lines setting A as the identity in Reset(), and edited the comment in the scenario. Let me know if you have any more suggestions. I appreciate your help!

sdunlap-afit · 2024-12-26T10:49:33Z

@Natsoulas On my last pass, I was primarily looking for references to AMatrix and missed a couple calls to setPropMatrix() in CoarseSunSensor::Reset().
Also, Magnetometer only calls setPropMatrix() in the constructor and doesn't have a getter/setter, so random walk can't be disabled.
Other than that, everything looks good to me. Thanks again for all your work on this!

schaubh

great progress, some additional feedback to address.

schaubh · 2024-12-27T15:38:50Z

src/simulation/sensors/imuSensor/imuSensor.h

@@ -73,19 +81,19 @@ class ImuSensor: public SysModel {
    bool NominalReady;                  //!< -- Flag indicating that system is in run
    Eigen::Matrix3d PMatrixAccel;   //!< [-] Cholesky-decomposition or matrix square root of the covariance matrix to apply errors with
 	Eigen::Matrix3d AMatrixAccel;   //!< [-] AMatrix that we use for error propagation
-	Eigen::Vector3d walkBoundsAccel;//!< [-] "3-sigma" errors to permit for states
+	Eigen::Vector3d walkBoundsAccel;  //!< [-] Total error bounds for accelerometer states. @warning Will be renamed to errorBoundsAccel in December 2025


Mm, this gives the user 1 year to know a change is coming, but not a true depreciation. Can we create the new errorBoundsAccel variable now as a privat variable, create the suitable setter and getter, document, but leave the public walkBoundsAccel. To see if the user sets it, could we default it to -1, and if anything else when we set errorBoundsAccel to this value? This way old code doesn't break, but users have 1 year to upgrade to the new setter and getter?

schaubh · 2024-12-27T15:39:40Z

src/simulation/sensors/imuSensor/imuSensor.h

+    void setAMatrixGyro(const Eigen::MatrixXd& propMatrix);
+    Eigen::MatrixXd getAMatrixAccel() const;
+    Eigen::MatrixXd getAMatrixGyro() const;
+    void setWalkBoundsAccel(const Eigen::Vector3d& bounds);


update to the new setter and getter names?

schaubh · 2024-12-27T15:41:41Z

src/simulation/sensors/imuSensor/imuSensor.cpp

+Eigen::Vector3d ImuSensor::getWalkBoundsGyro() const
+{
+    return this->walkBoundsGyro;
+}


Update the imuSensor.rst file to discuss how to properly set the noise values. You can note that the use of directly setting walkBoundsAccel is now deprecated.

schaubh · 2024-12-27T15:42:35Z

examples/scenarioGaussMarkovRandomWalk.py

@@ -54,6 +54,11 @@

 Both IMUs use the same process noise level (P Matrix) to ensure comparable noise magnitudes.

+Note that any sensors using the ``GaussMarkov`` noise model should be configured with
+user-defined configuration parameters such as ``walkBounds`` and ``AMatrix``. Otherwise,
+the default values will be used and they may not be appropriate for the sensor the user


By default we should have have no noise or random walk? Please ensure that signal is not modified unless the users sets these values.

schaubh · 2024-12-27T15:43:53Z

src/simulation/sensors/starTracker/starTracker.h

@@ -47,9 +47,12 @@ class StarTracker: public SysModel {
    void applySensorErrors();
    void computeTrueOutput();
    void computeQuaternion(double *sigma, STSensorMsgPayload *sensorValue);
-
+
+    void setAMatrix(const Eigen::MatrixXd& propMatrix);


should have a getAMatrix()?

schaubh · 2024-12-27T15:44:14Z

src/simulation/sensors/starTracker/starTracker.h

 public:
-    
+
    uint64_t sensorTimeTag;            //!< [ns] Current time tag for sensor out
    ReadFunctor<SCStatesMsgPayload> scStateInMsg;    //!< [-] sc input state message
    Message<STSensorMsgPayload> sensorOutMsg;   //!< [-] sensor output state message


need to update starTracker.rst as well to discuss how to set the noise values.

schaubh · 2024-12-27T15:45:48Z

src/simulation/sensors/magnetometer/magnetometer.cpp

+    nMatrix.setZero();
+    nMatrix(0,0) = this->senNoiseStd(0);
+    nMatrix(1,1) = this->senNoiseStd(1);
+    nMatrix(2,2) = this->senNoiseStd(2);


update module RST file on how to set noise values.

schaubh · 2024-12-27T15:46:38Z

src/simulation/sensors/coarseSunSensor/coarseSunSensor.h

@@ -99,6 +99,9 @@ class CoarseSunSensor: public SysModel {
    int                 CSSGroupID=-1;          //!< [-] (optional) CSS group id identifier, -1 means it is not set and default is used
    BSKLogger bskLogger;                        //!< -- BSK Logging

+    void setAMatrix(const Eigen::Matrix<double, -1, 1, 0, -1, 1>& propMatrix);


make the getAMatrix()?

schaubh · 2024-12-27T15:46:46Z

src/simulation/sensors/coarseSunSensor/coarseSunSensor.h

@@ -120,7 +124,7 @@ class CSSConstellation: public SysModel {
    void Reset(uint64_t CurrentClock);          //!< Method for reseting the module
    void UpdateState(uint64_t CurrentSimNanos); //!< @brief [-] Main update method for CSS constellation
    void appendCSS(CoarseSunSensor *newSensor); //!< @brief [-] Method for adding sensor to list
-    
+
 public:
    Message<CSSArraySensorMsgPayload> constellationOutMsg;  //!< [-] CSS constellation output message
    std::vector<CoarseSunSensor *> sensorList;    //!< [-] List of coarse sun sensors in constellation


update module RST file on how to set noise values.

Natsoulas added documentation Improvements or additions to documentation enhancement New feature or request labels Dec 21, 2024

Natsoulas self-assigned this Dec 21, 2024

Natsoulas requested a review from a team as a code owner December 21, 2024 19:52

Natsoulas linked an issue Dec 21, 2024 that may be closed by this pull request

Inconsistent sensor noise handling #867

Open

Natsoulas force-pushed the refactor-sensor-noise-handling branch 8 times, most recently from 34b0b7e to a7c5408 Compare December 23, 2024 01:43

imuSensor errorBounds future name change warning, getters and setters

b26e254

Natsoulas force-pushed the refactor-sensor-noise-handling branch from a7c5408 to fc98f12 Compare December 23, 2024 23:02

Natsoulas added 6 commits December 23, 2024 15:03

GM scenario rst update

b297976

starTracker refactor: getters + setters, initialize GM in constructor

ee6cf0d

simpleVoltEstimator: added setter + getter, initialize noise model in…

d99b5df

… constructor

Magnetometer refactor: initialize noise model in constructor, update …

16be932

…reset method

coarseSunSensor refactor: add a prop matrix setter + getter

74aea31

update known issues + release notes

e04558d

Natsoulas force-pushed the refactor-sensor-noise-handling branch from fc98f12 to e04558d Compare December 23, 2024 23:04

schaubh requested changes Dec 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor GaussMarkov sensor noise handling #881

Refactor GaussMarkov sensor noise handling #881

Natsoulas commented Dec 21, 2024

sdunlap-afit commented Dec 23, 2024

Natsoulas commented Dec 23, 2024

Natsoulas commented Dec 23, 2024

sdunlap-afit commented Dec 26, 2024

schaubh left a comment

schaubh Dec 27, 2024

schaubh Dec 27, 2024

schaubh Dec 27, 2024

schaubh Dec 27, 2024

schaubh Dec 27, 2024

schaubh Dec 27, 2024

schaubh Dec 27, 2024

schaubh Dec 27, 2024

schaubh Dec 27, 2024

Refactor GaussMarkov sensor noise handling #881

Are you sure you want to change the base?

Refactor GaussMarkov sensor noise handling #881

Conversation

Natsoulas commented Dec 21, 2024

Description

Verification

Documentation

Future work

sdunlap-afit commented Dec 23, 2024

Natsoulas commented Dec 23, 2024

Natsoulas commented Dec 23, 2024

sdunlap-afit commented Dec 26, 2024

schaubh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment