ENH: Enable only radial burning #815
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
caioessouza marked this conversation as resolved.
Show resolved
Hide resolved
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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 | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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): | ||
| """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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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 | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?