-
-
Notifications
You must be signed in to change notification settings - Fork 80
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
Add fuzz test, property-based test, and table test for PV function #100
base: main
Are you sure you want to change the base?
Add fuzz test, property-based test, and table test for PV function #100
Conversation
Here are some of the example errors caught in fuzz testing. We can handle or ignore these cases. InvalidOperation
ValueError
DivisionByZero
Overflow
|
aaccf23
to
34eebab
Compare
@@ -511,33 +513,29 @@ def ppmt(rate, per, nper, pv, fv=0, when='end'): | |||
return total - ipmt(rate, per, nper, pv, fv, when) | |||
|
|||
|
|||
def pv(rate, nper, pmt, fv=0, when='end'): | |||
def pv( | |||
rate: Union[int, float, Decimal, np.ndarray], |
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.
I added type hints here, but I see that they're not conventional in the repo. NP to remove the type hints if preferred for consistency; else, can keep it as optional.
Using Union
since the package supports Python 3.9. Starting in Python 3.10, we can do int|float|Decimal|np.ndarray
.
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.
Type hints are great. Adding type hints is actually part of what I'll be working on once I have the broadcasting done.
@@ -90,13 +93,167 @@ def test_decimal_with_when(self): | |||
|
|||
|
|||
class TestPV: |
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.
fwiw, I would remove the test classes and rely on function naming convention (ex: test_pv*
). Out of scope.
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.
I agree on principle, however having them in classes makes it easier to run groups of tests for each function.
}, | ||
} | ||
|
||
# Randomized input strategies for fuzz tests & property-based tests |
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.
Providing a "strategy" limits the values that the fuzzer (Hypothesis) will supply. The strategies that we re-use are defined here.
Thanks for taking the time to contribute. This looks excellent! I'm a bit short of time today/tomorrow. I'll try to get a full review done ASAP as there are a couple of other people touching these tests. FYI I'm currently working on a broadcasting + numba rewrite so many of these errors should be fixed after that. |
@@ -43,6 +43,7 @@ numba = "^0.58.1" | |||
|
|||
|
|||
[tool.poetry.group.test.dependencies] | |||
hypothesis = "^6.92.2" |
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.
I think adding hypothesis is the right idea here, however I don't have much experience with it. It might take me a while to thoroughly understand this
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.
No problem. Here are some links that I found helpful depending how far down the rabbit hole you want to go with it.
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.
Thanks! I've read the Hypothesis documentation and done some basic property based tests, but not much beyond that.
expected = float(npf.pv(rate=rate + 0.1, nper=nper, pmt=pmt, when=when)) | ||
|
||
# As interest rate increases, present value decreases | ||
assert round(result, 4) >= round(expected, 4) |
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.
I tried to use something like result >= pytest.approx(expected)
, but pytest.approx
doesn't work with floats and greater-than-or-equal-to operator.
remove logging
Decimal("-127128.1709461939327295222005"), | ||
) | ||
|
||
@pytest.mark.parametrize("test_case", test_cases.values(), ids=test_cases.keys()) |
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.
ids=
paramater gets us test case labels.
With ids
$ pytest tests/test_financial.py::TestPV::test_pv_examples -vv
tests/test_financial.py::TestPV::test_pv PASSED [ 9%]
tests/test_financial.py::TestPV::test_pv_decimal PASSED [ 18%]
tests/test_financial.py::TestPV::test_pv_examples[default_fv_and_when] PASSED [ 27%]
tests/test_financial.py::TestPV::test_pv_examples[specify_fv_and_when] PASSED [ 36%]
tests/test_financial.py::TestPV::test_pv_examples[when_1] PASSED [ 45%]
tests/test_financial.py::TestPV::test_pv_examples[when_1_and_fv_1000] PASSED [ 54%]
tests/test_financial.py::TestPV::test_pv_examples[fv>0] PASSED [ 63%]
tests/test_financial.py::TestPV::test_pv_examples[negative_rate] PASSED [ 72%]
tests/test_financial.py::TestPV::test_pv_examples[rates_as_array] PASSED [ 81%]
tests/test_financial.py::TestPV::test_pv_fuzz PASSED [ 90%]
tests/test_financial.py::TestPV::test_pv_interest_rate_sensitivity PASSED [100%]
Without ids
(no labels)
$ pytest tests/test_financial.py::TestPV::test_pv_examples -vv
tests/test_financial.py::TestPV::test_pv_examples[test_case0] PASSED [ 27%]
tests/test_financial.py::TestPV::test_pv_examples[test_case1] PASSED [ 36%]
tests/test_financial.py::TestPV::test_pv_examples[test_case2] PASSED [ 45%]
tests/test_financial.py::TestPV::test_pv_examples[test_case3] PASSED [ 54%]
tests/test_financial.py::TestPV::test_pv_examples[test_case4] PASSED [ 63%]
tests/test_financial.py::TestPV::test_pv_examples[test_case5] PASSED [ 72%]
tests/test_financial.py::TestPV::test_pv_examples[test_case6] PASSED [ 81%]
@paulzuradzki sorry for not being in touch. I moved states and then had some health issues. I'm back now. This work looks great and is definitely a candidate for merging. I'll get a formal review done this week if you are still interested in contributing. |
Description
This change request adds testing to the
pv
/ present value function.InvalidOperation
,TypeError
,DivisionByZero
,Overflow
.I opted to mirrornumpy
and return-0.0
. Arguably, we would want a runtime error and can ignore it in the fuzz tests.Context:
np.ndarray
of small dimensions, but this was a bit slow. I opted to test array-input directly (tests/test_financial.py::TestPV::test_pv_examples[rates_as_array
).Testing
If you want to select only the related tests, you can run them like so.