RocketPy icon indicating copy to clipboard operation
RocketPy copied to clipboard

ENH: Expansion of Encoders Implementation for Full Flights.

Open phmbressan opened this issue 1 year ago • 1 comments

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.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:

  • Custom Encoding for more types;
  • Customized behavior for some required classes with methods to_dict;
  • Support for Function encoding.

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_dict and (in the future) a from_dict method 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 _encoders private.
  • 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 handle functions/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.

phmbressan avatar Aug 30 '24 21:08 phmbressan

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.

Files with missing lines Patch % Lines
rocketpy/rocket/rocket.py 43.58% 22 Missing :warning:
rocketpy/environment/environment.py 59.61% 21 Missing :warning:
rocketpy/_encoders.py 78.18% 12 Missing :warning:
rocketpy/motors/tank_geometry.py 64.51% 11 Missing :warning:
...ocketpy/rocket/aero_surface/fins/free_form_fins.py 33.33% 6 Missing :warning:
rocketpy/rocket/parachute.py 71.42% 6 Missing :warning:
rocketpy/motors/tank.py 87.80% 5 Missing :warning:
...cketpy/rocket/aero_surface/fins/elliptical_fins.py 37.50% 5 Missing :warning:
rocketpy/rocket/components.py 37.50% 5 Missing :warning:
rocketpy/simulation/flight.py 37.50% 5 Missing :warning:
... and 9 more
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.

codecov[bot] avatar Aug 30 '24 21:08 codecov[bot]

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.md merging errors.

phmbressan avatar Nov 12 '24 09:11 phmbressan

Overall comments:

  • to_dict and from_dict methods 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...

Gui-FernandesBR avatar Nov 13 '24 21:11 Gui-FernandesBR

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.

phmbressan avatar Dec 07 '24 14:12 phmbressan