-
-
Notifications
You must be signed in to change notification settings - Fork 194
ENH: Discretized and No-Pickle Encoding Options #827
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?
Conversation
if kwargs.get("include_outputs", False): | ||
cl = self.cl | ||
if kwargs.get("discretize", False): | ||
cl = cl.set_discrete( |
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.
@MateusStano , could you give some insights if these boundary values make sense (angle of attack from -15 to 15 and mach 0 to 2)?
total_mass_flow_rate = self.total_mass_flow_rate | ||
center_of_propellant_position = self.center_of_propellant_position | ||
|
||
if discretize: |
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 admit this part of the code is not exactly pretty, but it was the way I found that better guarantees the necessary variables will be discrete.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #827 +/- ##
===========================================
+ Coverage 79.11% 80.14% +1.02%
===========================================
Files 96 98 +2
Lines 11575 12137 +562
===========================================
+ Hits 9158 9727 +569
+ Misses 2417 2410 -7 ☔ 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.
Pull Request Overview
This pull request introduces new encoding options—discretization and no‐pickle encoding—for RocketPy objects by refactoring the to_dict methods across many modules to accept keyword arguments (including include_outputs, discretize, and pickle_callables) instead of a fixed boolean flag. The changes affect multiple areas of the codebase including rocket components, aero surfaces, motors, math utilities, environment modules, and the custom JSON encoder.
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
rocketpy/rocket/components.py | Updated to_dict signature to accept **kwargs, removing the fixed include_outputs parameter. |
rocketpy/rocket/aero_surface/*.py | Refactored to_dict implementations within tail, rail_buttons, nose_cone, and fins modules to support new kwargs for discretization. |
rocketpy/motors/*.py | Updated to_dict methods in tank_geometry, tank, solid_motor, motor, liquid_motor, and hybrid_motor to use **kwargs with discretize handling. |
rocketpy/motors/fluid.py | Modified to_dict signature to accept **kwargs. |
rocketpy/mathutils/vector_matrix.py | Updated to_dict method to use **kwargs instead of a fixed include_outputs flag. |
rocketpy/mathutils/function.py | Refactored to_dict to incorporate discretize and pickle_callables flags and updated related logic. |
rocketpy/environment/environment.py | Added discretize handling for environmental data in to_dict. |
rocketpy/_encoders.py | Modified the custom JSON encoder to pass the updated kwargs to to_dict. |
CHANGELOG.md | Documented the enhancement with an entry for "Discretized and No-Pickle Encoding Options". |
Comments suppressed due to low confidence (2)
rocketpy/_encoders.py:50
- Consider updating the initializer docstring to clearly document the new 'discretize' and 'pickle_callables' kwargs, so users know how to control encoding behavior.
self.pickle_callables = kwargs.pop("pickle_callables", True)
rocketpy/motors/motor.py:1273
- Ensure that the updated kwargs usage (including 'discretize' and 'include_outputs') is accurately reflected in the method docstring and accompanying tests, to avoid potential confusion with legacy behavior.
if kwargs.get("discretize", False):
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 tests -m slow --runslow
) have passed locallyCHANGELOG.md
has been updatedCurrent behavior
The standard
RocketPyEncoder
currently does not support custom options for users/clients that may have the following needs:callable
definedFunctions
written out in a human readable format. Of course, this has the drawback that the originalcallable
cannot be exactly reproduced on decoding;New behavior
The added options
discretize
andallow_pickle
work as described above. The biggest challenge of the PR (which required a bit of hard coding) was setting discretization bounds for some of RocketPy callableFunctions
.Breaking change