-
-
Notifications
You must be signed in to change notification settings - Fork 202
ENH: Enable only radial burning #815
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
f048c56
4f32f88
550e149
7a4267a
95970b9
3287138
02de507
d97ba24
0cb0eb5
8de2ab4
e681877
4e4df54
c941070
2b3a15c
79e2a1c
3ecf1d7
65dfa23
79c066c
32f7711
f92cb64
d9d99dd
cb3c3a2
8ebbc90
292ab84
b99afb1
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 |
---|---|---|
|
@@ -193,8 +193,12 @@ class HybridMotor(Motor): | |
HybridMotor.reference_pressure : int, float | ||
Atmospheric pressure in Pa at which the thrust data was recorded. | ||
It will allow to obtain the net thrust in the Flight class. | ||
SolidMotor.only_radial_burn : bool | ||
If True, grain regression is restricted to radial burn only (inner radius growth). | ||
Grain length remains constant throughout the burn. Default is False. | ||
""" | ||
|
||
# pylint: disable=too-many-arguments | ||
def __init__( # pylint: disable=too-many-arguments | ||
self, | ||
thrust_source, | ||
|
@@ -216,6 +220,7 @@ def __init__( # pylint: disable=too-many-arguments | |
interpolation_method="linear", | ||
coordinate_system_orientation="nozzle_to_combustion_chamber", | ||
reference_pressure=None, | ||
only_radial_burn=True, | ||
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. This parameter needs to be documented in the 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. Check, please review again to make sure everything is ok |
||
): | ||
"""Initialize Motor class, process thrust curve and geometrical | ||
parameters and store results. | ||
|
@@ -313,6 +318,11 @@ class Function. Thrust units are Newtons. | |
"nozzle_to_combustion_chamber". | ||
reference_pressure : int, float, optional | ||
Atmospheric pressure in Pa at which the thrust data was recorded. | ||
only_radial_burn : boolean, optional | ||
If True, inhibits the grain from burning axially, only computing | ||
radial burn. If False, allows the grain to also burn | ||
axially. May be useful for axially inhibited grains or hybrid motors. | ||
Default is False. | ||
|
||
Returns | ||
------- | ||
|
@@ -364,6 +374,7 @@ class Function. Thrust units are Newtons. | |
interpolation_method, | ||
coordinate_system_orientation, | ||
reference_pressure, | ||
only_radial_burn, | ||
) | ||
|
||
self.positioned_tanks = self.liquid.positioned_tanks | ||
|
caioessouza marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ | |
|
||
|
||
@pytest.fixture | ||
def hybrid_motor(spherical_oxidizer_tank): | ||
def hybrid_motor(oxidizer_tank): | ||
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 don't oppose to it if needed, but any particular reason for changing the fixture name? 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. The spherical_oxidizer_tank fixture is broken. It's giving negative liquid heights. It need to be fixed in another issue. |
||
"""An example of a hybrid motor with spherical oxidizer | ||
tank and fuel grains. | ||
|
||
|
@@ -35,6 +35,6 @@ def hybrid_motor(spherical_oxidizer_tank): | |
grains_center_of_mass_position=-0.1, | ||
) | ||
|
||
motor.add_tank(spherical_oxidizer_tank, position=0.3) | ||
motor.add_tank(oxidizer_tank, position=0.3) | ||
|
||
return motor |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
from unittest.mock import patch | ||
|
||
|
||
@patch("matplotlib.pyplot.show") | ||
def test_solid_motor_info(mock_show, cesaroni_m1670): # pylint: disable=unused-argument | ||
"""Tests the SolidMotor.all_info() method. | ||
|
||
Parameters | ||
---------- | ||
mock_show : mock | ||
Mock of the matplotlib.pyplot.show function. | ||
cesaroni_m1670 : rocketpy.SolidMotor | ||
The SolidMotor object to be used in the tests. | ||
""" | ||
assert cesaroni_m1670.info() is None | ||
assert cesaroni_m1670.all_info() is 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.
There can be made some discussion here on the severity of it, but this is likely a breaking change: the default of this attribute should be
False
, otherwise all the current code with hybrid motors would have its behavior changed. Do you agree @Gui-FernandesBR ?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 don't think this change would break anything, but I think it can change the propellant mass evolution with time. Anyway, this behaviour is more similar to reality, so in my conception even if it changes, it's not necessarily a bad thing. Do you agree @Gui-FernandesBR @MateusStano ? If you think it's a problem, I can change.
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 agree it changes results, and I understand it changes for the better.
@phmbressan don't you think we could proceed with this?