RocketPy-Team / RocketPy

Next generation High-Power Rocketry 6-DOF Trajectory Simulation
https://docs.rocketpy.org/
MIT License
570 stars 137 forks source link

ENH: Function Validation Rework & Swap `np.searchsorted` to `bisect_left` #582

Closed MateusStano closed 2 months ago

MateusStano commented 3 months ago

Pull request type

Checklist

Current behavior

  1. The input parameters of the Funciton class are all checked in a very disorganized way. This would lead to redundant checks and overall slowing down of the class
  2. all interpolations used in get_value_opt were using np.searchsorted, which was slow

New behavior

  1. Validation has been reworked as to follow the order: source -> interpolation and extrapolation -> inputs, outputs and title. The get_value_opt function is now modularized and is defined according to the current interpolation and extrapolation
  2. bisect_left is now used instead of np.searchsorted. This cuts the execution time by half in a u_dot_generalized call.

The Function class should behave exactly the same as before.

Breaking change

Additional information

Gui-FernandesBR commented 3 months ago

@MateusStano I made all the commentaries that I found important to be done in this first review. Overall, code is much better and intuitive. Let's follow the discussions in the next days so we can merge this PR really soon.

Gui-FernandesBR commented 3 months ago

Also, @MateusStano can we make all the new methods completely private (i.e. use def __your_private_method) instead of using a single underscore?

These new methods won't be used out of the Function class, plus using double score seems better, it allows python to mangle the methods.

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 88.77005% with 21 lines in your changes are missing coverage. Please review.

:exclamation: No coverage uploaded for pull request base (mnt/flight-simulation-speed-up@939d294). Click here to learn what that means.

:exclamation: Current head 29bb5fa differs from pull request most recent head 00a3c1f. Consider uploading reports for the commit 00a3c1f to get more accurate results

Files Patch % Lines
rocketpy/mathutils/function.py 88.77% 21 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## mnt/flight-simulation-speed-up #582 +/- ## ================================================================= Coverage ? 72.86% ================================================================= Files ? 59 Lines ? 9547 Branches ? 0 ================================================================= Hits ? 6956 Misses ? 2591 Partials ? 0 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Gui-FernandesBR commented 3 months ago

@MateusStano good work solving the majority of commentaries!

Could you add tests for the lines missing coverage?

Gui-FernandesBR commented 3 months ago

Also, @MateusStano could you please add your PR to the CHANGELOG?