-
-
Notifications
You must be signed in to change notification settings - Fork 166
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
BUG: Motor method 'export_eng' for liquid motors bug fix. (solves #473) #559
Conversation
I'd like to add that Issue #473 requests the implementation of tests. However, I'd like to request permission to not write tests for this method for now. My plan is to refactor the unit tests for the liquid_motor module later today, and I intend to include the tests for this method as part of that refactor. |
.eng files documentation: https://www.thrustcurve.org/info/raspformat.html Saving that for reference. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #559 +/- ##
===========================================
+ Coverage 72.47% 72.60% +0.12%
===========================================
Files 59 59
Lines 9567 9584 +17
===========================================
+ Hits 6934 6958 +24
+ Misses 2633 2626 -7 ☔ View full report in Codecov by Sentry. |
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 know the code works, but I think there is a little code repetition that perhaps could be avoid, thus improving readability.
I will add a code suggestion here. I know it is also changing some other stuffs like the "with context" and adopting the f-strings instead of .format(), but I hope you can understand it. Feel free to accept the suggestions, while in this very same PR or in other.
def export_eng(self, file_name, motor_name):
"""Exports thrust curve data points and motor description to
.eng file format. A description of the format can be found
here: http://www.thrustcurve.org/raspformat.shtml
Parameters
----------
file_name : string
Name of the .eng file to be exported. E.g. 'test.eng'
motor_name : string
Name given to motor. Will appear in the description of the
.eng file. E.g. 'Mandioca'
Returns
-------
None
"""
# Open file
with open(file_name, "w") as file:
# Write first line
def get_attr_value(obj, attr_name, multiplier=1):
return multiplier * getattr(obj, attr_name, 0)
grain_outer_radius = get_attr_value(self, "grain_outer_radius", 2000)
grain_number = get_attr_value(self, "grain_number", 1000)
grain_initial_height = get_attr_value(self, "grain_initial_height")
grain_separation = get_attr_value(self, "grain_separation")
grain_total = grain_number * (grain_initial_height + grain_separation)
if grain_outer_radius == 0 or grain_total == 0:
warnings.warn(
"The motor object doesn't have some grain-related attributes. "
"Using zeros to write to file."
)
file.write(
f"{motor_name} {grain_outer_radius:3.1f} {grain_total:3.1f} 0 "
f"{self.propellant_initial_mass:2.3} "
f"{self.propellant_initial_mass:2.3} RocketPy\n"
)
# Write thrust curve data points
for time, thrust in self.thrust.source[1:-1, :]:
file.write(f"{time:.4f} {thrust:.3f}\n")
# Write last line
file.write(f"{self.thrust.source[-1, 0]:.4f} {0:.3f}\n")
return None
@Gui-FernandesBR Much better, thanks. Committed your solution |
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.
Nice, well done @lucasfourier !!
Can you please update the CHANGELOG.md file with this PR?
It is important since it makes it easier for users and contributors to see precisely what changes have been made between each version of the project.
Done 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.
Thanks for updating the CHANGELOG, but it was wrong though.
Please add your PR to the [Unreleased] - yyyy-mm-dd
section, under the Fixed.
The v1.2.0 (where you added) is already released.
Fixed 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.
Still requiring changes on the CHANGELOG. Please take a look at other sections of the file so you can keep the same pattern
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.
This is better:
## [Unreleased] - yyyy-mm-dd
<!-- These are the changes that were not release yet, please add them correctly.
Attention: The newest changes should be on top -->
### Added
### Changed
### Fixed
- BUG: export_eng 'Motor' method would not work for liquid motors. [#559](https://github.com/RocketPy-Team/RocketPy/pull/559)
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.
Got it!
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
As described in #473, 'export_eng' method does not work for liquid motors since it does not have grain related attributes.
New behavior
The Motor method 'export_eng' now works for liquid motors and the .eng file is created. For liquid motors, the first two parameters written to the file are now zeros, since those are grain related attributes.
Breaking change
Additional information
Below is the '.eng' file generated for the liquid motor defined in the SEB_liquid_motor.ipynb notebook.