Skip to content
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: Function Validation Rework & Swap np.searchsorted to bisect_left #582

Merged

Conversation

MateusStano
Copy link
Member

@MateusStano MateusStano commented Mar 28, 2024

Pull request type

  • Code changes (bugfix, features)
  • Code maintenance (refactoring, formatting, tests)

Checklist

  • Docs have been reviewed and added / updated
  • Lint (black rocketpy/ tests/) has passed locally
  • All tests (pytest tests -m slow --runslow) have passed locally

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

  • Yes
  • No

Additional information

  • There is an attribute called __img_dim__ that was never used and was always equal to 1. I changed it so its defined at the __init__ for backward compatibility purposes, but it is still never used
  • Further optimizations can be done in __mul__, __sub__, __truediv__ and other operations by avoiding going through all the validation checks when re-initializing the Functions

@MateusStano MateusStano requested a review from a team as a code owner March 28, 2024 18:14
@MateusStano MateusStano mentioned this pull request Mar 28, 2024
7 tasks
@MateusStano MateusStano added the Function Everything related to the Function class label Mar 28, 2024
@Gui-FernandesBR
Copy link
Member

@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
Copy link
Member

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.

@MateusStano MateusStano changed the title ENH: Function Falidation Rework & Swap np.searchsorted to bisect_left ENH: Function Validation Rework & Swap np.searchsorted to bisect_left Mar 29, 2024
Copy link

codecov bot commented Mar 30, 2024

Codecov Report

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

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

❗ 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 ⚠️
Additional details and impacted files
@@                        Coverage Diff                        @@
##             mnt/flight-simulation-speed-up     #582   +/-   ##
=================================================================
  Coverage                                  ?   72.86%           
=================================================================
  Files                                     ?       59           
  Lines                                     ?     9547           
  Branches                                  ?        0           
=================================================================
  Hits                                      ?     6956           
  Misses                                    ?     2591           
  Partials                                  ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

MateusStano and others added 5 commits March 30, 2024 19:28
Co-authored-by: Gui-FernandesBR <63590233+Gui-FernandesBR@users.noreply.github.com>
Co-authored-by: Gui-FernandesBR <63590233+Gui-FernandesBR@users.noreply.github.com>
rocketpy/mathutils/function.py Show resolved Hide resolved
rocketpy/mathutils/function.py Outdated Show resolved Hide resolved
rocketpy/mathutils/function.py Show resolved Hide resolved
@Gui-FernandesBR
Copy link
Member

@MateusStano good work solving the majority of commentaries!

Could you add tests for the lines missing coverage?

@Gui-FernandesBR
Copy link
Member

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

@Gui-FernandesBR Gui-FernandesBR merged commit f03adeb into mnt/flight-simulation-speed-up Apr 4, 2024
9 checks passed
@Gui-FernandesBR Gui-FernandesBR deleted the enh/function-validation-rework branch April 4, 2024 17:32
@Gui-FernandesBR Gui-FernandesBR linked an issue Apr 19, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Function Everything related to the Function class
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

ENH: _check_user_input method being called multiple times
3 participants