-
-
Notifications
You must be signed in to change notification settings - Fork 189
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
Conversation
fd2ca79
to
6f3086c
Compare
Codecov ReportAttention: Patch coverage is
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. 🚀 New features to boost your workflow:
|
There was a problem hiding this 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
Performance BenchmarksFunction Composition (
|
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.
87ddd0f
to
1f89b44
Compare
There was a problem hiding this 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)
ae07fcb
to
d7a3491
Compare
Pull request type
Checklist
pytest tests -m slow --runslow
) have passed locallyCHANGELOG.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 NDFunctions
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:
self._domain
andself._image
for array manipulation, instead of unidimensionalself.x_array
andself.y_array
;lambda
;eval
, it can easily be converted into anexception
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 ofmul
from #785 is non commutative when dealing with 1D*2D, which is fixed by this PR.Breaking change