ENH: Enable only radial burning #815
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -216,6 +216,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. 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 There was a problem hiding this comment. 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. I agree it changes results, and I understand it changes for the better. @phmbressan don't you think we could proceed with this? There was a problem hiding this comment. This parameter needs to be documented in the There was a problem hiding this comment. Check, please review again to make sure everything is ok |
||
| ): | ||
| """Initialize Motor class, process thrust curve and geometrical | ||
| parameters and store results. | ||
|
|
@@ -364,6 +365,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(oxidizer_tank): | ||
|
There was a problem hiding this comment. I don't oppose to it if needed, but any particular reason for changing the fixture name? There was a problem hiding this comment. The spherical_oxidizer_tank fixture is broken. It's giving negative liquid heights. It need to be fixed in another issue. There was a problem hiding this comment. @caioessouza what issue are you referring to? There was a problem hiding this comment. There's no issue for it yet, but the spherical_oxidizer_tank fixture is returning negative liquid heights for this burning duration. |
||
| """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(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 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,7 +56,7 @@ def test_hybrid_motor_basic_parameters(hybrid_motor): | |
| assert hybrid_motor.liquid.positioned_tanks[0]["position"] == 0.3 | ||
|
|
||
|
|
||
| def test_hybrid_motor_thrust_parameters(hybrid_motor, oxidizer_tank): | ||
| """Tests the HybridMotor class thrust parameters. | ||
|
|
||
| Parameters | ||
|
|
@@ -77,13 +77,13 @@ def test_hybrid_motor_thrust_parameters(hybrid_motor, spherical_oxidizer_tank): | |
| * GRAIN_INITIAL_HEIGHT | ||
| * GRAIN_NUMBER | ||
| ) | ||
| initial_oxidizer_mass = oxidizer_tank.fluid_mass(0) | ||
| initial_mass = initial_grain_mass + initial_oxidizer_mass | ||
|
|
||
| expected_exhaust_velocity = expected_total_impulse / initial_mass | ||
| expected_mass_flow_rate = -expected_thrust / expected_exhaust_velocity | ||
| expected_grain_mass_flow_rate = ( | ||
| expected_mass_flow_rate - oxidizer_tank.net_mass_flow_rate | ||
| ) | ||
|
|
||
| assert pytest.approx(hybrid_motor.thrust.y_array) == expected_thrust.y_array | ||
|
|
@@ -100,7 +100,7 @@ def test_hybrid_motor_thrust_parameters(hybrid_motor, spherical_oxidizer_tank): | |
| ) == expected_grain_mass_flow_rate(t) | ||
|
|
||
|
|
||
| def test_hybrid_motor_center_of_mass(hybrid_motor, oxidizer_tank): | ||
| """Tests the HybridMotor class center of mass. | ||
|
|
||
| Parameters | ||
|
|
@@ -110,25 +110,25 @@ def test_hybrid_motor_center_of_mass(hybrid_motor, spherical_oxidizer_tank): | |
| spherical_oxidizer_tank : rocketpy.SphericalTank | ||
| The SphericalTank object to be used in the tests. | ||
| """ | ||
| oxidizer_mass = oxidizer_tank.fluid_mass | ||
| grain_mass = hybrid_motor.solid.propellant_mass | ||
|
|
||
| propellant_balance = grain_mass * GRAINS_CENTER_OF_MASS_POSITION + oxidizer_mass * ( | ||
| OXIDIZER_TANK_POSITION + oxidizer_tank.center_of_mass | ||
| ) | ||
| balance = propellant_balance + DRY_MASS * CENTER_OF_DRY_MASS | ||
|
|
||
| propellant_center_of_mass = propellant_balance / (grain_mass + oxidizer_mass) | ||
| center_of_mass = balance / (grain_mass + oxidizer_mass + DRY_MASS) | ||
|
|
||
| for t in np.linspace(0, BURN_TIME, 100): | ||
| assert pytest.approx( | ||
| hybrid_motor.center_of_propellant_mass(t) | ||
| ) == propellant_center_of_mass(t) | ||
| assert pytest.approx(hybrid_motor.center_of_mass(t)) == center_of_mass(t) | ||
|
|
||
|
|
||
| def test_hybrid_motor_inertia(hybrid_motor, oxidizer_tank): | ||
| """Tests the HybridMotor class inertia. | ||
|
|
||
| Parameters | ||
|
|
@@ -138,8 +138,8 @@ def test_hybrid_motor_inertia(hybrid_motor, spherical_oxidizer_tank): | |
| spherical_oxidizer_tank : rocketpy.SphericalTank | ||
| The SphericalTank object to be used in the tests. | ||
| """ | ||
| oxidizer_mass = oxidizer_tank.fluid_mass | ||
| oxidizer_inertia = oxidizer_tank.inertia | ||
| grain_mass = hybrid_motor.solid.propellant_mass | ||
| grain_inertia = hybrid_motor.solid.propellant_I_11 | ||
| propellant_mass = oxidizer_mass + grain_mass | ||
|
|
@@ -153,7 +153,7 @@ def test_hybrid_motor_inertia(hybrid_motor, spherical_oxidizer_tank): | |
| oxidizer_mass | ||
| * ( | ||
| OXIDIZER_TANK_POSITION | ||
| + oxidizer_tank.center_of_mass | ||
| - hybrid_motor.center_of_propellant_mass | ||
| ) | ||
| ** 2 | ||
|
|
@@ -170,9 +170,46 @@ def test_hybrid_motor_inertia(hybrid_motor, spherical_oxidizer_tank): | |
| + DRY_MASS * (-hybrid_motor.center_of_mass + CENTER_OF_DRY_MASS) ** 2 | ||
| ) | ||
|
|
||
| for t in np.linspace(0, BURN_TIME, 100): | ||
| assert pytest.approx(hybrid_motor.propellant_I_11(t)) == propellant_inertia(t) | ||
| assert pytest.approx(hybrid_motor.I_11(t)) == inertia(t) | ||
|
|
||
| # Assert cylindrical symmetry | ||
| assert pytest.approx(hybrid_motor.propellant_I_22(t)) == propellant_inertia(t) | ||
|
|
||
|
|
||
| def test_hybrid_motor_only_radial_burn_behavior(hybrid_motor): | ||
| """ | ||
| Test if only_radial_burn flag in HybridMotor propagates to its SolidMotor | ||
| and affects burn_area calculation. | ||
| """ | ||
| motor = hybrid_motor | ||
|
|
||
| # Activates the radial burning | ||
| motor.solid.only_radial_burn = True | ||
| assert motor.solid.only_radial_burn is True | ||
|
Comment on lines
+189
to
+190
There was a problem hiding this comment. This is quite dangerous. I did not test nor exhaustively review the code, but here are some of my comments that I believe are relevant (feel free to correct me on that) if you want to support setting (post instantiation) this attribute:
On the top of my head, I see two possible solutions here:
class SolidMotor(Motor):
def __init__(self, ..., only_radial_burn=False):
...
self._only_radial_burn = only_radial_burn
@property
def only_radial_burn(self):
return self._only_radial_burn
@only_radial_burn.setter
def only_radial_burn(self, value):
self._only_radial_burn = value
self.evaluate_geometry()
class HybridMotor(Motor):
def __init__(self, ..., only_radial_burn=False):
...
self._only_radial_burn = only_radial_burn
@property
def only_radial_burn(self):
return self._only_radial_burn
@only_radial_burn.setter
def only_radial_burn(self, value):
self.solid.only_radial_burn = value
reset_funcified_method(self)
class SolidMotor(Motor):
def __init__(self, ..., only_radial_burn=False):
...
self._only_radial_burn = only_radial_burn
@property
def only_radial_burn(self):
return self._only_radial_burn@caioessouza could you confirm this is the case, i.e., toggling this attribute would cause the defined attributes from I, personally, have a slight preference for option 2, since it avoids complicating more this already delicate interaction between the motor types. Do you have any suggestions @MateusStano or @Gui-FernandesBR ? There was a problem hiding this comment. I think the best option is to make This would make life much easier. If you want to modify the value of only_radial_burn after instanciating the object, you should instantiate another object There was a problem hiding this comment. I've just implemented option 2. I agree that makes more sense, because it's a characteristic of each motor, so it's not mutable in reality anyway. There was a problem hiding this comment. When I added this class SolidMotor(Motor): It broke a lot of tests, maybe I've done something wrong. |
||
|
|
||
| # Calculates the expected initial area | ||
| burn_area_radial = ( | ||
| 2 | ||
| * np.pi | ||
| * (motor.solid.grain_inner_radius(0) * motor.solid.grain_height(0)) | ||
| * motor.solid.grain_number | ||
caioessouza marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ) | ||
|
|
||
| assert np.isclose(motor.solid.burn_area(0), burn_area_radial, atol=1e-12) | ||
|
|
||
| # Deactivates the radial burning and recalculate the geometry | ||
| motor.solid.only_radial_burn = False | ||
| motor.solid.evaluate_geometry() | ||
| assert motor.solid.only_radial_burn is False | ||
|
|
||
| # In this case the burning area also considers the bases of the grain | ||
| inner_radius = motor.solid.grain_inner_radius(0) | ||
| outer_radius = motor.solid.grain_outer_radius | ||
| burn_area_total = ( | ||
| burn_area_radial | ||
| + 2 * np.pi * (outer_radius**2 - inner_radius**2) * motor.solid.grain_number | ||
| ) | ||
| assert np.isclose(motor.solid.burn_area(0), burn_area_total, atol=1e-12) | ||
| assert motor.solid.burn_area(0) > burn_area_radial | ||
Uh oh!
There was an error while loading. Please reload this page.