-
-
Notifications
You must be signed in to change notification settings - Fork 211
Enh/custom warning no motor parachute aerosurface #871
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?
Changes from all commits
1d7b2f2
4eff12e
a4f44c4
c00e2dd
1d75f80
c787b36
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a rocket without parachute is something quite common. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Following up on issue #285 and your comment: I wanted to clarify: for rockets without parachutes, should the custom warning be changed to a notice/info message instead, or should it only trigger under specific conditions?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should never raise an warning for "parachuteless rockets", so to say |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -384,6 +384,32 @@ def __init__( # pylint: disable=too-many-statements | |||||||||||||||||||
| self.prints = _RocketPrints(self) | ||||||||||||||||||||
| self.plots = _RocketPlots(self) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def _check_missing_components(self): | ||||||||||||||||||||
| """Check if the rocket is missing any essential components and issue a warning. | ||||||||||||||||||||
|
|
||||||||||||||||||||
| This method verifies whether the rocket has the following key components: | ||||||||||||||||||||
| - motor | ||||||||||||||||||||
| - aerodynamic surface(s) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| If any of these components are missing, a single warning message is issued | ||||||||||||||||||||
| listing all missing components. This helps users quickly identify potential | ||||||||||||||||||||
| issues before running simulations or analyses. | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Notes: | ||||||||||||||||||||
| - The warning uses Python's built-in `warnings.warn` function. | ||||||||||||||||||||
|
Comment on lines
+398
to
+399
|
||||||||||||||||||||
| Notes: | |
| - The warning uses Python's built-in `warnings.warn` function. | |
| Notes | |
| ----- | |
| - The warning uses Python's built-in `warnings.warn` function. | |
| Returns | |
| ------- | |
| None |
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.
True.
Copilot
AI
Nov 19, 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 condition not self.aerodynamic_surfaces already covers empty lists, making the explicit len(self.aerodynamic_surfaces) == 0 check redundant. Simplify to just if not self.aerodynamic_surfaces:.
| if not self.aerodynamic_surfaces or len(self.aerodynamic_surfaces) == 0: | |
| if not self.aerodynamic_surfaces: |
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.
true
Copilot
AI
Nov 19, 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 '[WARNING]' prefix in the warning message is redundant since warnings.warn() already identifies messages as warnings. Remove the prefix to avoid duplication: f'Rocket has no {component_list} defined.'
| f"[WARNING] Rocket has no {component_list} defined.", UserWarning | |
| f"Rocket has no {component_list} defined.", UserWarning |
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.
true
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -10,7 +10,9 @@ | |||
|
|
||||
|
|
||||
| @patch("matplotlib.pyplot.show") | ||||
| def test_elliptical_fins(mock_show, calisto_robust, calisto_trapezoidal_fins): # pylint: disable=unused-argument | ||||
| def test_elliptical_fins( | ||||
| mock_show, calisto_robust, calisto_trapezoidal_fins | ||||
| ): # pylint: disable=unused-argument | ||||
| test_rocket = calisto_robust | ||||
| calisto_robust.aerodynamic_surfaces.remove(calisto_trapezoidal_fins) | ||||
| test_rocket.add_elliptical_fins(4, span=0.100, root_chord=0.120, position=-1.168) | ||||
|
|
@@ -370,6 +372,39 @@ def test_add_motor(calisto_motorless, cesaroni_m1670): | |||
| assert center_of_mass_motorless is not center_of_mass_with_motor | ||||
|
|
||||
|
|
||||
| def test_check_missing_all_components(calisto_motorless): | ||||
| """Tests the _check_missing_components method for a Rocket with no components.""" | ||||
| with pytest.warns(UserWarning) as record: | ||||
| calisto_motorless._check_missing_components() | ||||
|
|
||||
| assert len(record) == 1 | ||||
| msg = str(record[0].message) | ||||
| assert "motor" in msg | ||||
| assert "aerodynamic surfaces" in msg | ||||
|
|
||||
|
|
||||
| def test_check_missing_some_components(calisto): | ||||
| """Tests the _check_missing_components method for a Rocket missing some components.""" | ||||
| calisto.parachutes = [] | ||||
|
||||
| calisto.parachutes = [] |
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.
true
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.
are you sure?
# If any warning occurs, pytest will fail automatically
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.
Thank you for the review!You're correct.By default, pytest only displays warnings but doesn't fail.I will updated the test.
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.
changelog seems weird @phmbressan
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.
Thanks for your feedback. Could you please clarify which part of the changelog seems weird? I want to make sure my addition follows the proper format.
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.
your added line is listed along together other changes that have already been deployed
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.
I can double-check later, but this branch might not be up to date with develop (v1.11), which is why the CHANGELOG seems outdated. Could you @C8H10O2 try rebasing this branch with the current develop so that all conflicts are sorted out?
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.
Understood, I’ll take care of it. Thanks for pointing it out.