-
-
Notifications
You must be signed in to change notification settings - Fork 167
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
ENH: Expansion of Encoders Implementation for Full Flights. #679
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #679 +/- ##
===========================================
- Coverage 76.42% 76.34% -0.08%
===========================================
Files 95 96 +1
Lines 11090 11460 +370
===========================================
+ Hits 8475 8749 +274
- Misses 2615 2711 +96 ☔ 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.
Nothing to complain about, good work.
I have just noticed that I made a mistake when merging CHANGELOG conflicts for this PR. I will fix it before merging. This does not block reviewing.
|
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.
Overall comments:
to_dict
andfrom_dict
methods are missing docstrings in all classes
I strongly advise to not include dosctrings yet. Also, they exists mainly for private reasons: we need them in order have the JSONEncoder working. Btw the methods are so simple that they don't even need docstrings. But that's a discussion for future sessions... |
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.
One last test we could run is: Create a Monte Carlo simulation with 100 simulations and verify if the output .txt files are still readable enough.
c8e1682
to
7834947
Compare
Good point. I have added some customization options to MonteCarlo so that one can choose whether to include all |
fe27050
to
f1b57ef
Compare
MNT: Add encoding feature to CHANGELOG. BUG: add dill to the requirements file.
ENH: extend encoding and decoding to Liquid and Hybrid. MNT: correct decoding of liquid and hybrid motors. STY: solve pylint remarks. MNT: adapt encoding to new post merge attributes. MNT: restore typo to correct values on flight test. ENH: add option for including outputs on JSON export. TST: add tests for motor encoding. DOC: Improve docstrings of encoders signature. MNT: Make no output encoding the default. MNT: Standardize include outputs parameter. DOC: Correct phrasing and typos of encoders docstring. MNT: Correct json export environment naming.
006f6a9
to
c79930b
Compare
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.
@phmbressan just a few more conversations need to be solved.
The PR looks good btw.
DEV: fix CHANGELOG MNT: reposition barometric height as env json output. MNT: Allow for encoding customization of MonteCarlo.
c79930b
to
c389519
Compare
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, amazing work @phmbressan .
@GabrielBarberini should be interested in this feature. Please notice this is not a part of the public API!!
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
Currently the
_encoding
module is just a stub and cannot still encode full simulations into a JSON like format.New behavior
This PR expand this module bringing the following:
to_dict
;Function
encoding.This PR also introduces decoding functionalities. The way this works is:
to_dict
orfrom_dict
method, they are used to perform the (de)encoding;Breaking change
Additional information
There is much to discuss here on the implementation and maintainability side of things. Main discussion points:
to_dict
and (in the future) afrom_dict
method to some (or all) rocketpy classes?_encoders
private.simplejson
: brings some more types and speed into encoding, but does not help much with custom types;jsonpickle
: really great for the typing handler (already writes by default each object type) and is able to serialize. The problem is that is does not handlefunctions/lambdas
and I did not find a good way to implement custom handlers for our use case;pickle / dill
: the most plug and play solution, however the output is not human readable and not generally compatible between different rocketpy versions.Of course, this is what I understood upon researching and testing, feel free to make any comments or suggest other modules.
Remarks and Future Considerations
Here are some of the steps that could be improved:
AirBrakes
+Controllers
and new classes.Some important remarks: