-
-
Notifications
You must be signed in to change notification settings - Fork 197
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?
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:
|
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.
Are we missing something in the HybridMotor class here? If we want to define a a HybridMotor with only radial burning garin how would we do that currently?
I recommend modifyng the hybrid class in another PR, it's easier to reivew it. |
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.
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 |
The "only_radial_burning" parameter was added and set as default on the hybrid class. Also, the description of the new parameter was updated and 2 coments about derivatives set to zero were added.
…aioessouza/RocketPy into enh/enable-only-radial-burning
only_radial_burn argument order corrected
The solid motor integrations test was created and the solid and hybrid motors unit tests were modified to add checks to the new only_radial_burning class and parameters. Also a correction was made in the hybrid unit tests to fix the test time.
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.
Code looks better now, I think it is just a matter of taking a deeper look into the calculations and checking that everything is correct.
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.
Pull Request Overview
This PR enables users to restrict solid motor grain burning to only the radial direction by introducing a new only_radial_burn
parameter. This enhancement allows for more accurate simulation of axially inhibited grains or hybrid motors where axial burning is not desired.
- Added
only_radial_burn
boolean parameter to both SolidMotor and HybridMotor classes with appropriate default values - Modified geometry evaluation logic to conditionally disable axial burning based on the new parameter
- Implemented comprehensive test coverage for the new functionality
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
rocketpy/motors/solid_motor.py | Adds only_radial_burn parameter and implements conditional burning logic in geometry calculations |
rocketpy/motors/hybrid_motor.py | Propagates only_radial_burn parameter to HybridMotor with default value of True |
tests/unit/test_solidmotor.py | Adds unit tests for radial burn parameter effects and geometry evaluation |
tests/unit/test_hybridmotor.py | Updates test time ranges and adds comprehensive tests for hybrid motor radial burn behavior |
tests/integration/test_solidmotor.py | Adds new integration test file for SolidMotor info methods |
CHANGELOG.md | Documents the new feature addition |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Comments checked and resolved. @Gui-FernandesBR please check the CHANGELOG.md to see if it's correct now.
Pull request type
Checklist
black rocketpy/ tests/
) has passed locallypytest tests -m slow --runslow
) have passed locallyCHANGELOG.md
has 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.