Skip to content

ENH: support for ND arithmetical operations for Function. #810

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

Merged
merged 5 commits into from
May 15, 2025

Conversation

phmbressan
Copy link
Collaborator

@phmbressan phmbressan commented Apr 22, 2025

Pull request type

  • Code changes (bugfix, features)

Checklist

  • Tests for the changes have been added (if needed)
  • Docs have been reviewed and added / updated
  • All tests (pytest tests -m slow --runslow) have passed locally
  • CHANGELOG.md has been updated (if relevant)

Current behavior

PR #785 introduced the possibility of 2D Function multiplication. The intent of this PR is to generalize this to the other arithmetical operations and to ND Functions for better scalability (e.g. so as not keep adding if blocks for every dimension, like 3D, 4D etc).

New behavior

The generalization is done with the following changes:

  • Use the self._domain and self._image for array manipulation, instead of unidimensional self.x_array and self.y_array;
  • Multivariate lambda expressions are generated to allow callable operations:
    • The implementation is less trivial, requiring dynamically building a multivariate lambda;
    • This PR implementation worked very well on testing, but, if it does not hold review scrutinity due the use of eval, it can easily be converted into an exception and disallowing it.

Tests were introduced for both existing 2D operations and for the new generalized ones.

Overall the structure of the arithmetic methods of Function got a bit simpler and more regular. Another important notice, is that the current multivariable implementation of mul from #785 is non commutative when dealing with 1D*2D, which is fixed by this PR.

Breaking change

  • Yes
  • No

@phmbressan phmbressan added the Function Everything related to the Function class label Apr 22, 2025
@phmbressan phmbressan added this to the Release v1.X.0 milestone Apr 22, 2025
@phmbressan phmbressan self-assigned this Apr 22, 2025
@phmbressan phmbressan requested a review from a team as a code owner April 22, 2025 09:27
@phmbressan phmbressan force-pushed the enh/function-nd-arith branch 2 times, most recently from fd2ca79 to 6f3086c Compare April 22, 2025 10:12
Copy link

codecov bot commented Apr 22, 2025

Codecov Report

Attention: Patch coverage is 93.13305% with 16 lines in your changes missing coverage. Please review.

Project coverage is 80.14%. Comparing base (4df0b38) to head (01d0f9d).
Report is 20 commits behind head on develop.

Files with missing lines Patch % Lines
rocketpy/mathutils/function.py 93.13% 16 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #810      +/-   ##
===========================================
+ Coverage    79.11%   80.14%   +1.02%     
===========================================
  Files           96       98       +2     
  Lines        11575    12030     +455     
===========================================
+ Hits          9158     9641     +483     
+ Misses        2417     2389      -28     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-project-automation github-project-automation bot moved this from Backlog to Next Version in LibDev Roadmap Apr 22, 2025
Copy link
Member

@MateusStano MateusStano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check execution speed of the code? With Monte Carlo sims especially

The operations seems different so I just want to make sure it is close to how it was before

@phmbressan
Copy link
Collaborator Author

Can you check execution speed of the code? With Monte Carlo sims especially

The operations seems different so I just want to make sure it is close to how it was before

Performance Benchmarks

Function Composition (f + g) and Evaluation (h(x))

Test Before After
f + g (lambda) 16.2 μs ± 1.24 μs 17.5 μs ± 0.41 μs
h(3) (lambda eval) 1.59 μs ± 0.013 μs 725 ns ± 5.47 ns
f + g (array function) 110 μs ± 0.82 μs 102 μs ± 1.01 μs
h(2.5) (array eval) 3.85 μs ± 0.045 μs 3.74 μs ± 0.068 μs

Monte Carlo Simulation

Metric Before After
Avg Time per Iteration 1.337 s 1.276 s
Total Simulations 250 250
Total Wall Time 334.3 s 319.0 s

Overall the results are about the same speed, mostly within run to run variance. The exception is the Function lambda evaluation that is now consistently faster and I don't know why, still checking.

@phmbressan phmbressan force-pushed the enh/function-nd-arith branch 2 times, most recently from 87ddd0f to 1f89b44 Compare April 22, 2025 16:13
@Gui-FernandesBR Gui-FernandesBR requested a review from Copilot April 23, 2025 15:57
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR generalizes arithmetical operations for the Function class from 2D to ND and improves internal consistency by using the set_source method.

  • Refactors source setting by replacing direct assignment with set_source.
  • Adds extensive parametrized tests covering add, sub, mul, div, pow, and mod operations for both 2D and ND functions.
  • Updates CHANGELOG.md to reflect the new ND arithmetic support.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
tests/unit/test_function.py Updates to test cases including replacing source assignment with set_source and adding parametrized tests for ND arithmetic operations.
CHANGELOG.md Added an entry for ND arithmetic support in the Function class.
Comments suppressed due to low confidence (1)

tests/unit/test_function.py:907

  • [nitpick] When using lambda functions in ND arithmetic tests, ensure that the lambda and other operand functions have matching arity and consistent naming conventions to avoid ambiguity in multi-dimensional operations.
func_lambda = Function(lambda x, y, z: x + y + z)

@phmbressan phmbressan force-pushed the enh/function-nd-arith branch from ae07fcb to d7a3491 Compare May 2, 2025 19:37
@MateusStano MateusStano merged commit 6e78dfc into develop May 15, 2025
31 of 52 checks passed
@MateusStano MateusStano deleted the enh/function-nd-arith branch May 15, 2025 14:07
@github-project-automation github-project-automation bot moved this from Next Version to Closed in LibDev Roadmap May 15, 2025
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.

3 participants