Updated irr function [issue 98] #99
Conversation
calculate IRR by calculating all its real roots
|
Sorry I found that I made a trival mistake: should be |
Corrected variable name `values` from ``cash_flow
- test_financial.py: Altered test cases - _financial.py: Added optional argument `raise_exceptions`
|
Updated tests/test_financial.py and nump_financial/_financial.py
|
There was a problem hiding this comment.
This looks pretty good.
Could you add a test case where it would normally have returned a “worse” outcome, e.g. the one in your example?
I'm a bit hesitant to choose a solution for the user: I guess we are doing that implicitly anyway with the iterative solver, so this is an improvement.
tests/test_financial.py
Outdated
|
|
||
| with pytest.raises(npf.IterationsExceededError): | ||
| npf.irr(cashflows, maxiter=1, raise_exceptions=True) | ||
| npf.irr(cashflows, raise_exceptions=True) |
There was a problem hiding this comment.
There's a missing blank line here at the end of the file
numpy_financial/_financial.py
Outdated
| # if the signs of IRR solutions are not the same, first filter potential IRR | ||
| # by comparing the total positive and negative cash flows. | ||
| if not same_sign: | ||
| pos = sum(values[values>0]) | ||
| neg = sum(values[values<0]) | ||
| if pos > neg: |
There was a problem hiding this comment.
I don't understand why you are trying to filter the by the total cashflow. Is this just some heuristic you have picked or is there a more formal process going on here?
There was a problem hiding this comment.
This is just some heuristic I have picked. It works when there are real roots with different signs.
I can give you a simple example and walk you through it:
real roots = -0.5 and 0.1
net sum of the cash flows = +10000
As the net sum > 0, we can kind of exclude the possibility that IRR is -0.5 and keep only 0.1.
There was a problem hiding this comment.
That makes sense. Thanks for clarifying.
numpy_financial/_financial.py
Outdated
| else: | ||
| IRR = IRR[IRR < 0] |
There was a problem hiding this comment.
Should this be indented further?
numpy_financial/_financial.py
Outdated
| g = 1 + guess | ||
|
|
||
| g = np.roots(values) | ||
| IRR = np.real(g[np.isreal(g)]) - 1 |
There was a problem hiding this comment.
Nitpick: I don't like having all capital variable names, could you rename it to something meaningful?
There was a problem hiding this comment.
IRR -> internal_rate_return
There was a problem hiding this comment.
That would work. Or maybe something like irr_ if you wanted to be brief. The rest of the code seems to be brief, although I prefer being explicit in general.
|
Interestingly, I just pumped into a situation like this. The real roots are 0.0 and 0.2136. Considering the sum of cashflow = 0, did Excel give an incorrect solution? But I personally prefer selecting the smallest root in terms of its magnitude more. So, I will prefer 0.0 rather than 0.2136. I think we do have to choose a solution for users implicitly. But maybe also include arguments allowing them to choose their options? Like Any suggestion? |
indentation
As you have previously noted, there is more than 1 valid solution. I believe that Excel uses an iterative solver to find a solution, with the initial guess being 10%. My guess is that, as both are mathematically valid, Excel simply chose the first solution it converged too.
I don't think we have to decide a solution for the user explicitly. We could do it via our existing iterative solver. Although that has weaknesses (as you already know).
I'm -1 on giving more options to |
- internal_rate_of_return -> eirr - abs_internal_rate_of_return -> abs_eirr
|
I've finally updated the code. here is what I did:
because I don't see why it shows an error here even if the error is actually small here. therefore, i removed the test case that I made previously. |
I just realized someone had previously made a pull request on changing the _test_financial.py and removed all the assert_almost_equal function, making my original function fail on those newly added sophisicated test cases, with a very small abs difference like 9e-16 which is not 0. So, I've removed all the test cases that I added previously. |
|
Changes look good. I've asked a friend from industry to try and poke some holes into the new method, but I don't expect there to be anything seriously wrong here. Thanks for taking the initiative! |
Np! My pleasure! I always enjoy making things like this. |
That's great to hear. There are plenty of things that will still need to be done on NumPy-financial if you are interested As an aside, for the future, could you please follow the guide writing you commit message from the NumPy docs? |
Yea thanks for letting me know |
| d_npv = npv_.deriv() | ||
| g = 1 + guess | ||
|
|
||
| g = np.roots(values) |
There was a problem hiding this comment.
From the roots docs:
This forms part of the old polynomial API. Since version 1.4, the new polynomial API defined in numpy.polynomial is preferred. A summary of the differences can be found in the transition guide.
However I'm not sure if it's worth to construct a whole Polynomial object just for this one calculation...
There was a problem hiding this comment.
I'm currently re-writing some of the functions to use numba, it looks like np.roots is supported, but the new Polynomial API is not. So I'd prefer this version.
|
Merged, Thanks @yatshunlee for taking the time to contribute. |
calculate IRR by calculating all its roots and select the most reasonable IRR among all the real roots
https://numpy.org/doc/stable/reference/generated/numpy.roots.html