BUG: removed unnecesary warning from financial.pmt method #6073
Conversation
Previous implementation would cause divide by zero and hence warning since "fact" variable was calculated for all rates (both zero and non-zero) but then zero rates would be masked with a where equality. Changed implementation to apply non-zero rates using "np.divide( numerator, denominator, where=mask_of_zero_values)"and then apply zero rates with "np.copyto". Also updated documentation to show exact formulas that are being calculated. Done with help of Sebastian Berg in Austin,TX! Fixes issue numpy#5046
| return -(fv + pv*temp) / fact | ||
|
|
||
| # use different payment if rate == 0 | ||
| zero_rate_mask = rate == 0 |
There was a problem hiding this comment.
Please put parens around the == expression to make it easier to read: zero_rate_mask = (rate == 0)
There was a problem hiding this comment.
Ok updated the code per your suggestion and ran "git pull --force"
On Mon, Jul 13, 2015 at 3:53 PM, Nathaniel J. Smith <
notifications@github.com> wrote:
In numpy/lib/financial.py
#6073 (comment):@@ -208,12 +215,15 @@ def pmt(rate, nper, pv, fv=0, when='end'):
"""
when = _convert_when(when)
(rate, nper, pv, fv, when) = map(np.asarray, [rate, nper, pv, fv, when])
- temp = (1 + rate)**nper
- mask = (rate == 0.0)
- np.copyto(rate, 1.0, where=mask)
- z = np.zeros(np.broadcast(rate, nper, pv, fv, when).shape)
- fact = np.where(mask != z, nper + z, (1 + rate_when)_(temp - 1)/rate + z)
- return -(fv + pv*temp) / fact
use different payment if rate == 0
- zero_rate_mask = rate == 0
Please put parens around the == expression to make it easier to read: zero_rate_mask
= (rate == 0)—
Reply to this email directly or view it on GitHub
https://github.com/numpy/numpy/pull/6073/files#r34510377.
Previous implementation would cause divide by zero and hence warning since
"fact" variable was calculated for all rates (both zero and non-zero) but then
zero rates would be masked with a where equality.
Changed implementation to apply non-zero rates using "np.divide( numerator,
denominator, where=mask_of_zero_values)"and then apply zero rates with "np.copyto".
Also updated documentation to show exact formulas that are being calculated.
Done with help of Sebastian Berg in Austin,TX!
7/13/15 Added in parenthesis for reability per Nathaniel Smith comment e.g.
zero_rate_mask = (rate == 0)
Fixes issue numpy#5046
| payment = np.asarray(np.divide(-(fv + pv*compounded_rate)*rate, | ||
| (1 + rate*when)*(compounded_rate - 1), | ||
| where=~zero_rate_mask)) | ||
| np.copyto(payment, -(fv + pv)/nper, where=zero_rate_mask) |
There was a problem hiding this comment.
(Sorry, got interrupted half-way through reviewing :-))
I'd put spaces around most or all of these binary operators (e.g. (1 + rate * when) * (compounded_rate - 1)), though in this case it's more a matter of personal taste than rigid policy. (In general spaces around operators is PEP8's recommendation, but there's an escape clause for higher-priority operators that could apply here if one wanted.)
|
Also, is there a test for this function? If not it would be good to add one (esp. checking both rate = 0 and not). Otherwise LGTM (= looks good to me). |
Conflicts: numpy/lib/financial.py
|
Ok! Added in spaces around binary operators as suggested. There are pmt test in test_financial.py but the tests did not try to catch def test_pmt(self): On Mon, Jul 13, 2015 at 5:56 PM, Nathaniel J. Smith <
|
|
You should check that removing |
|
☔ The latest upstream changes (presumably #8050) made this pull request unmergeable. Please resolve the merge conflicts. |
|
This was fixed long ago by a similar pr: gh-5577 (some of the doc changes could possible be moved, but I do not think it is worth keeping this open for). Thanks! |
Previous implementation would cause divide by zero and hence warning since
"fact" variable was calculated for all rates (both zero and non-zero) but then
zero rates would be masked with a where equality.
Changed implementation to apply non-zero rates using "np.divide( numerator,
denominator, where=mask_of_zero_values)"and then apply zero rates with "np.copyto".
Also updated documentation to show exact formulas that are being calculated.
Done with help of Sebastian Berg in Austin,TX!
Fixes issue #5046