Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
schaubh
added
documentation
schaubh
left a comment
schaubh
left a comment
There was a problem hiding this comment.
The original reference-radius issue is valid, and retaining the coefficient model's radius while leaving the body's physical radius unchanged is the right fix. A compiled comparison of the original and proposed implementations against an independent J2 calculation confirmed the correction.
Requesting changes for the new GM fallback, which can make translational gravity and gravity-gradient torque use different gravitational parameters. There is also a smaller Doxygen documentation requirement to address; both findings are attached inline.
Validation: rebuilt the affected native target; all 31 gravity-effector tests passed; both touched headers compiled independently. An additional integrated spacecraft reproduction exposed the GM inconsistency described below.
| // The body gravitational parameter is used for the field so that it is consistent with | ||
| // the rest of the simulation (e.g. orbital element conversions). | ||
| if (body.mu > 0.0) { | ||
| this->muBody = body.mu; | ||
| } |
There was a problem hiding this comment.
[P2] Keep the effective GM consistent across gravity consumers
When body.mu remains at its default zero, this new fallback preserves the file GM only in spherHarm.muBody. GravityEffector::computeGravityField() still publishes body.mu as the planet's .mu state-engine property, which GravityGradientEffector uses. The result is nonzero orbital acceleration but zero gravity-gradient torque.
I reproduced this with a spacecraft using GGM03S truncated to degree zero, position [4.0e6, -3.0e6, 5.5e6] m, identity attitude, and inertia diag([500, 800, 350]) kgm^2. With body.mu = 0, the model GM is 3.986004415e14 m^3/s^2 and the published GM is zero; torque is [0, 0, 0]. Setting body.mu to the file GM leaves acceleration unchanged but gives torque [3.91313456072929e-4, 1.7391709158796851e-4, -1.8972773627778374e-4] Nm.
Please synchronize the effective GM across consumers, or require a positive body GM instead of accepting this partial fallback. Add an integrated regression test: the new test_spherical_harmonics_keep_file_mu_without_body_mu only checks the model's GM and therefore endorses the inconsistent configuration.
There was a problem hiding this comment.
Just pushed a fix, @carlo98 , please see if you agree.
| * The attribute `radEquator` is kept if it is positive, for example when it was | ||
| * loaded from the coefficient file, since the normalized coefficients are only | ||
| * valid with the reference radius of their model. Otherwise, it is read from | ||
| * the `radEquator` of the given `GravBodyData`. | ||
| * | ||
| * The attribute `muBody` is replaced by the `mu` of the given `GravBodyData` | ||
| * if that value is positive. Otherwise, the current `muBody` is kept. |
There was a problem hiding this comment.
[P3] Add the required Doxygen parameter and return sections
This materially updated API comment falls under AGENTS.md's documentation requirement, but the overload still declares an unnamed const GravBodyData& parameter and provides no @param or @return sections. Generating Doxygen XML for this header confirms that neither section is present for this overload.
Please name the declaration parameter body and add @brief, @param body, and @return documentation, retaining the explanation of radius/GM precedence and the optional initialization error.
There was a problem hiding this comment.
Just pushed a commit to address this.
- Require finite, positive body GM for spherical harmonics. - Verify acceleration and gravity-gradient torque against GM saved before initialization. - Align integrator SWIG vector declarations with shared bindings. - Add import-order regression tests and update documentation. Validation: 117 gravity, MuJoCo, binding, and integrator tests passed.
| // Match swig_common_model.i's template arguments, including the inner allocator, | ||
| // so SWIG uses the same type descriptors for DoubleVector and MultiArray. | ||
| %template() std::vector<double, std::allocator<double>>; | ||
| %template() std::vector<std::vector<double, std::allocator<double>>>; |
There was a problem hiding this comment.
Should the same change be applied also in sphericalHarmonicsGravityModel.i? Adding it also to the relevant unittests?
|
|
||
| @pytest.mark.parametrize("import_order", [ | ||
| ("gravityEffector", "svIntegrators"), | ||
| ("svIntegrators", "gravityEffector"), |
There was a problem hiding this comment.
If the comment "Should the same change be applied also in sphericalHarmonicsGravityModel.i?" is ok, then the following lines are needed here
("sphericalHarmonicsGravityModel", "svIntegrators"), ("svIntegrators", "sphericalHarmonicsGravityModel"),
| nodes = [0.0, 0.5] # [-] | ||
|
|
||
| def coefficients(): | ||
| model = gravityEffector.SphericalHarmonics() |
There was a problem hiding this comment.
If the comment "Should the same change be applied also in sphericalHarmonicsGravityModel.i?" is ok, then the following line is needed here
for model in (gravityEffector.SphericalHarmonics(), SphericalHarmonicsGravityModel()):
| importlib.import_module("Basilisk.simulation." + name) | ||
|
|
||
| from Basilisk.architecture import swig_common_model | ||
| from Basilisk.simulation import gravityEffector, spacecraft, svIntegrators |
There was a problem hiding this comment.
If the comment "Should the same change be applied also in sphericalHarmonicsGravityModel.i?" is ok, then the following line should be added here
from Basilisk.simulation.sphericalHarmonicsGravityModel import SphericalHarmonicsGravityModel
This branch has not been deployed
2 participants
Description
GravBodyData.useSphericalHarmonicsGravityModel(file, maxDeg)loadsspherHarm.radEquatorandspherHarm.muBodyfrom the first row of the coefficient file, butSphericalHarmonicsGravityModel::initializeParameters(const GravBodyData&)replaced both with theradEquatorandmuof the gravity body, without a warning.Normalized coefficients are only valid with the reference radius of their model. When the radii differ, every term of degree
nis scaled by(R_body / R_file)^n.Changes:
SphericalHarmonicsGravityModel::initializeParameters(const GravBodyData&)keeps the modelradEquatorwhen it is positive (loaded from the file or set by the user).body.radEquatoris used only when the model has no radius, for example when the coefficients are set directly.body.mu, which keeps it consistent with the rest of the simulation. Ifbody.muis not positive, the model keeps its ownmuBodyinstead of producing a zero field. A mismatch betweenbody.muand the GM of the file is documented.radEquatorof the body is also used by other modules.Important: Scripts that set
planet.radEquatorto change the field must now setplanet.spherHarm.radEquator.Verification
src/simulation/dynamics/gravityEffector/_UnitTest/test_gravity_model_configuration.py.Documentation
SphericalHarmonicsGravityModel::initializeParametersand of theGravBodyDatamu/radEquatormembers describes the behaviour.useSphericalHarmonicsGravityModel(gravityEffector.i) andloadFromFile(sphericalHarmonicsGravityModel.i).gravityEffector.rst.bskKnownIssues.rst.Future work
body.mudiffers from the GM of the coefficient file.scenarioGroundDownlink.pyandscenarioSmallBodyNavUKF.pyload spherical harmonics files but have no test insrc/tests.