-
-
Notifications
You must be signed in to change notification settings - Fork 208
ENH: unstable rocket error #764
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
Changes from all commits
e914b4f
7228980
269b1e6
f5d2559
04d2354
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| """ | ||
| Custom exceptions for RocketPy library. | ||
| """ | ||
|
|
||
|
|
||
| class RocketPyException(Exception): | ||
| """All RocketPy custom exceptions should inherit from this class.""" | ||
|
|
||
|
|
||
| class UnstableRocketError(RocketPyException): | ||
| """Raised when the rocket jas negative static margin.""" | ||
|
|
||
| def __init__(self, stability_margin): | ||
| self.stability_margin = stability_margin | ||
| self.message = ( | ||
| "Rocket is unstable. Initial Static Margin is " | ||
| f"{stability_margin} calibers, this can lead to eternal loop simulation." | ||
| ) | ||
|
|
||
| def __str__(self): | ||
| return self.message | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |
| import simplekml | ||
| from scipy.integrate import BDF, DOP853, LSODA, RK23, RK45, OdeSolver, Radau | ||
|
|
||
| from ..exceptions import UnstableRocketError | ||
| from ..mathutils.function import Function, funcify_method | ||
| from ..mathutils.vector_matrix import Matrix, Vector | ||
| from ..plots.flight_plots import _FlightPlots | ||
|
|
@@ -639,6 +640,7 @@ def __init__( # pylint: disable=too-many-arguments,too-many-statements | |
| self.__init_solution_monitors() | ||
| self.__init_equations_of_motion() | ||
| self.__init_solver_monitors() | ||
| self.__validate_rocket_static_margin() | ||
|
|
||
| # Create known flight phases | ||
| self.flight_phases = self.FlightPhases() | ||
|
|
@@ -664,6 +666,15 @@ def __repr__(self): | |
| f"name= {self.name})>" | ||
| ) | ||
|
|
||
| def __validate_rocket_static_margin(self): | ||
| """ | ||
| Avoids running a flight simulation if the rocket stability margin is | ||
| negative. This is a common mistake that can lead to unstable flights, | ||
| which usually runs indefinitely. | ||
| """ | ||
| if (s := self.rocket.static_margin(0)) < 0: | ||
| raise UnstableRocketError(s) | ||
|
|
||
|
Comment on lines
+669
to
+677
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. I think this error check might not be suitable right now. I see a few points as to why:
Maybe raising a warning is the best we can do?
Collaborator
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. I agree with the first point you raised: not all rockets that are instantaneously unstable are necessarily unsimulatable. A possible compromise would be either a warning or letting the user set a variable, such as
Collaborator
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. I like the Idea of having a flag with default to false (so current API is respected). Then we can simply turn It on for infinity
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. Would And if the default is And if the default is
Collaborator
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. Sorry, I meant default
Member
Author
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. Converting from an error to a warning is the easiest option right now. However, I think we should better understand the cases where an unstable rocket can fly. If setting ALLOW_UNSTABLE to false, we should describe this behavior in the error message.
Collaborator
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. I think setting It to IMHO a simple warning would not be easy to consume for safe guarding purposes
Member
Author
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. I understand. My point is that maybe we should not keep the current behavior at all.
Member
Author
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. I will think more about this during the week |
||
| # pylint: disable=too-many-nested-blocks, too-many-branches, too-many-locals,too-many-statements | ||
| def __simulate(self, verbose): | ||
| """Simulate the flight trajectory.""" | ||
|
|
||
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.
has*