ENH: Expansion of Encoders Implementation for Full Flights.
Pull request type
- [X] Code changes (bugfix, features)
Checklist
- [X] Tests for the changes have been added (partially done)
- [ ] Docs have been reviewed and added / updated (future PR)
- [X] Lint (
black rocketpy/ tests/) has passed locally - [X] All tests (
pytest tests -m slow --runslow) have passed locally - [X]
CHANGELOG.mdhas 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:
- Custom Encoding for more types;
- Customized behavior for some required classes with methods
to_dict; - Support for
Functionencoding.
Breaking change
- [ ] Yes
- [X] No
Additional information
There is much to discuss here on the implementation and maintainability side of things. Main discussion points:
- Should we add a
to_dictand (in the future) afrom_dictmethod to some (or all) rocketpy classes?- My Opinion: I think adding the methods to everything might be the only way out. But there is some work and maintainability considerations.
- The current status of the encoding can be fully deserialized?
- My Opinion: All the information is there, but the typing should be improved to know which objects to decode (deserialize) into. For that reason, I kept the
_encodersprivate.
- My Opinion: All the information is there, but the typing should be improved to know which objects to decode (deserialize) into. For that reason, I kept the
- Are there any other modules to make our work easier?
- From my research, I have tested the following:
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/lambdasand 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.
- From my research, I have tested the following:
Of course, this is what I understood upon researching and testing, feel free to make any comments or suggest other modules.
Codecov Report
Attention: Patch coverage is 70.58824% with 115 lines in your changes missing coverage. Please review.
Project coverage is 76.34%. Comparing base (
83aa20e) to head (c389519). Report is 10 commits behind head on develop.
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
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
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.
- [x] Fix
CHANGELOG.mdmerging errors.
Overall comments:
to_dictandfrom_dictmethods are missing docstrings in all classes
I strongly advise to not include dosctrings yet. We don't really know if these methods will really be used.
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...
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.
Good point. I have added some customization options to MonteCarlo so that one can choose whether to include all Function data or keep the way it is currently on develop that is only the Function string.