-
-
Notifications
You must be signed in to change notification settings - Fork 194
FIX: changes Generic Motor exhaust velocity to cached property #497
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
FIX: changes Generic Motor exhaust velocity to cached property #497
Conversation
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.
Thank you for going through the journey to discover the bottleneck in this class. I was postponing for some time. Great finds!
Won't even start about snowball effect that the Function
is currently generating that slows down the code so much. I will only say that the sooner this is fixed, the better.
This reverts commit ecf536d.
…icMotor to avoid lambdas
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #497 +/- ##
==========================================
- Coverage 70.95% 70.92% -0.04%
==========================================
Files 55 55
Lines 9262 9262
==========================================
- Hits 6572 6569 -3
- Misses 2690 2693 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ 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 swear that I tried but I honestly have no idea of how you discovered this error in such a chain of calls.
Before merging, do you see any possibility for new unit tests here? It's quite complicated because the error was only happening due to a "snowball" effect. |
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.
LGTM
Pull request type
Checklist
black rocketpy/ tests/
) has passed locallypytest --runslow
) have passed locallyCHANGELOG.md
has been updated (if relevant)Current behavior
GenericMotor.exhaust_velocity
is being decorated byfuncify_method
, which makes it (a float essentially) into alambda
definedFunction
. This causes everything that depends on it algebraically to becomelambda
definedFunctions
as well.More specifically, this contaminates
GenericMotor.mass_flow_rate
, which in turn contaminatesGenericMotor.propellant_mass
(which actually includes anintegral_function
of thelambda
GenericMotor.exhaust_velocity
🫣). Well, in the end, this contaminatesRocket.total_mass
, which is used extensively by theFlight
class during a simulation.In summary, this use of
funcify_method
causes theFlight
class to take forever (literally) to run.New behavior
What can I say? Changing the decorator from
funcify_method
tocached_property
makes the sun shine once again, solving everything.Breaking change