-
-
Notifications
You must be signed in to change notification settings - Fork 194
Enh/zero inertia tensor check #833
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
base: develop
Are you sure you want to change the base?
Conversation
- ENH adding a small check to avoid singularity of inertia input in rocket class
- MNT: correction to input of inertia tensor into abs(). Created an inertia matrix using Matrix right before the singularity check.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #833 +/- ##
==========================================
+ Coverage 80.06% 80.08% +0.01%
==========================================
Files 98 98
Lines 12042 12044 +2
==========================================
+ Hits 9642 9645 +3
+ Misses 2400 2399 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 pull request introduces a singularity check for the rocket's inertia tensor and updates the version parsing logic. Key changes include updating the version comparison in utilities.py using packaging.version.parse, adding a check for a singular inertia tensor in rocket.py, and synchronizing the project version info across configuration and documentation files.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
rocketpy/utilities.py | Updated version check to use packaging.version.parse for comparisons |
rocketpy/rocket/rocket.py | Added inertia tensor singularity check using a matrix determinant comparison |
pyproject.toml | Updated project version to 1.10.0 |
docs/user/installation.rst | Updated installation instructions to reflect latest version |
docs/conf.py | Updated Sphinx release version to 1.10.0 |
CHANGELOG.md | Added changelog entries for version 1.10.0 |
Comments suppressed due to low confidence (1)
rocketpy/utilities.py:756
- The use of version("rocketpy") may result in a NameError because the 'version' function is not imported; consider replacing it with the appropriate version string or variable (e.g., a version attribute) to ensure the comparison works correctly.
):
[self.I_12_without_motor, self.I_22_without_motor, self.I_23_without_motor], | ||
[self.I_13_without_motor, self.I_23_without_motor, self.I_33_without_motor], | ||
]) # Initial Inertia Tensor determinant singularity check | ||
if abs(inertia_matrix) == 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.
Using abs(inertia_matrix) to check for a singular matrix might not correctly compute the determinant; it is recommended to use the proper determinant method (e.g., inertia_matrix.det()) for an accurate singularity check.
if abs(inertia_matrix) == 0: | |
if inertia_matrix.det() == 0: |
Copilot uses AI. Check for mistakes.
Pull request type
Checklist
black rocketpy/ tests/
) has passed locallypytest tests -m slow --runslow
) have passed locallyCHANGELOG.md
has been updated (if relevant)Current behavior
As of now there are no checks on the singularity of input inertia tensor. This can create some issues where the simulation may still run even if the input inertia tensor was singular and give ill defined results. This was pointed by an user on discord but could not be recreated
New behavior
A simple check has been added to rocket initialisation for checking the input dry inertia.
Breaking change
Additional information
For future this check can be extended to dynamically testing the singularity of inertia tensor. Need for this is to be evaluated.