ENH: Enable only radial burning #815
Conversation
only_radial_burn optional parameter added and used in the functions related to the grain regression as a conditon to change the applied equations. Still need to update the comment section, the CHANGELOG and maybe the dict related functions, but not sure about the last one yet.
The new parameter of the SolidMotor class was removed from super, since it's not on the Motor class. The dict functions were updated to take this new parameter into acount. Also the comments about the SolidMotor class parameters were updated. Still need to do some tests running the code to be sure everything is ok, then I can open the PR.
Corrected some minor code erros, like calling evaluate geometry before defining self.only_radial_burn. Also had to change the grain_height_derivative to zero in the case of only radial burn, it was leading to some physical incoherences, because the grain was still burning axially. The last tests to evaluate the physical behaviour went pretty well, so I'll open the PR.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #815 +/- ##
===========================================
+ Coverage 79.11% 80.08% +0.96%
===========================================
Files 96 98 +2
Lines 11575 12042 +467
===========================================
+ Hits 9158 9644 +486
+ Misses 2417 2398 -19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I recommend modifyng the hybrid class in another PR, it's easier to reivew it. |
| @@ -314,6 +315,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. Otherwise, if False, allows the grain to also burn | |||
There was a problem hiding this comment.
Redundant
| radial burn. Otherwise, if False, allows the grain to also burn | |
| radial burn. Otherwise, allows the grain to also burn |
or
| radial burn. Otherwise, if False, allows the grain to also burn | |
| radial burn. If False, allows the grain to also burn |
There was a problem hiding this comment.
It is important to add at least an integration test to this new implementation.
There was a problem hiding this comment.
Great PR.
I recommend keeping this PR to solidmotor (possibly renaming it).
Later you could extend the feature to also cover hybrid motors.
Also, I believe two other steps are important:
- Update the Motor page on our documentation to include this new feature
- Include new tests!!
|
But if you really want to include hybrid motors here, not a problem |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces an enhancement to the SolidMotor class allowing users to enable only radial burning by setting the "only_radial_burn" parameter.
- Added the "only_radial_burn" parameter in the constructor with accompanying documentation.
- Updated geometry_dot, geometry_jacobian, and burn_area methods to reflect the new burning mode.
- Modified serialization methods and the changelog to include the new parameter.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| rocketpy/motors/solid_motor.py | Added "only_radial_burn" parameter and updated geometry-related methods accordingly. |
| CHANGELOG.md | Documented the enhancement to enable only radial burning. |
| grain_outer_radius**2 | ||
| - grain_inner_radius**2 | ||
| + grain_inner_radius * grain_height | ||
| if self.only_radial_burn: |
There was a problem hiding this comment.
Consider adding an inline comment to explain why grain_height_derivative is set to 0 when only_radial_burn is active to improve clarity for future maintainers.
Copilot uses AI. Check for mistakes.
| grain_outer_radius**2 | ||
| - grain_inner_radius**2 | ||
| + grain_inner_radius * grain_height | ||
| if self.only_radial_burn: |
There was a problem hiding this comment.
Add a comment in the Jacobian computation explaining the rationale for zeroing out the height derivatives when only_radial_burn is True.
Copilot uses AI. Check for mistakes.
Pull request type
Checklist
black rocketpy/ tests/) has passed locallypytest tests -m slow --runslow) have passed locallyCHANGELOG.mdhas been updated (if relevant)Current behavior
Rocketpy SolidMotor class simulates the grain regression either radially and axially by default.
This PR is related to the following issue:
ENH: Enable only radial burning #801
New behavior
Now it's possible to the user to change from the default behavior to a only radial regression by setting the new "only_radial_burning" optional parameter as True in the SolidMotor class.
Breaking change
Additional information
Here is an example of the comparison between the default burning and the only radial one applied to the getting_started.ipynb simulation. The pytest tests haven't passed locally because of a windows problem.