Skip to content

Conversation

WarrenWeckesser
Copy link
Member

I tried to preserve the commit history of 'financial.py' and 'test_financial.py'.

I added a few standard files, including a very basic 'setup.py'. It works well enough to install a package that passes the (updated) tests:

$ ~/mc37numpy/bin/pytest 
================================================== test session starts ===================================================
platform darwin -- Python 3.7.3, pytest-4.6.2, py-1.8.0, pluggy-0.12.0
rootdir: /Users/warren/repos/git/forks/numpy-financial
collected 26 items                                                                                                       

test_financial.py ..........................                                                                       [100%]

=============================================== 26 passed in 0.13 seconds ================================================

There is a lot more basic configuration to do, but I'd like to verify that this initial structure looks good before proceeding.

Also, any Sphinx gurus, distutils gurus, CI gurus, PyPI gurus, etc., are encouraged to contribute pull requests so we can get this package into shape as quickly.

teoliphant and others added 30 commits September 27, 2019 11:27
…vative and takes into account re-investing profits and expense of financing losses.
…ite to simplify "if __name__ == '__main__'" boilerplate code in test modules. Removed numpy/testing/pkgtester.py since it just consisted of an import statement after porting SciPy r4424. Allow numpy.*.test() to accept the old keyword arguments (but issue a deprecation warning when old arguments are seen). numpy.*.test() returns a test result object as before. Fixed typo in distutils doc.
…up and (somewhat) standardize test module imports. Removed unneeded reload calls.
…instead of explicit imports or dependency on the local scope where the doctest is defined..
…of payment) functions. Added doctests and unit tests.
The ipmt function was also fixed to handle broadcasting. The tests
were improved and extended to cover the broadcasting capability.
This should be harmless, as we already are division clean. However,
placement of this import takes some care. In the future a script
can be used to append new features without worry, at least until
such time as it exceeds a single line. Having that ability will
make it easier to deal with absolute imports and printing updates.
Kai-Striega and others added 8 commits September 27, 2019 11:27
``np.npv`` calculates the present value of a series of cashflows
starting in the present (t=0). Another common definition of NPV
uses the future cashflows i.e. starting at the end of the first
period.

The difference in definition can lead to confusion for users moving
between NumPy and other projects [1] and adds complexity for
maintainers [2]. This commit adds a warning to the ``npv``
function's documentation and adds an example on how to find the npv
of a project using the alternate definition.

[1] https://github.com/numpy/numpy/issue/10877
[2] numpy/numpy#3346
The internal rate of return (irr) is defined as the rate of return
required for the net present values of a series of cashflows to be
zero. i.e the lowest rate of return required for a project to break
even.

This is currently checked by refering to the example output from
the ``irr`` and ``npv`` function documentation. This commit adds a
test to confirm the identity holds.
* Eliminate intermediate 'lib' directory.
* Rename financial.py to _financial.py
* Add __init__.py
Copy link
Member

@Kai-Striega Kai-Striega left a comment

Choose a reason for hiding this comment

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

LGTM just a few comments:

  • We're missing an AUTHORS.txt
  • Using import numpy rather than import numpy as np is a good precaution, and will probably save some headaches later (thanks)
  • numpy_financial/tests/test_financial.py is ~ 70 lines longer than numpy/lib/tests/test_financial.py. As far as I can tell this is due to the extra imports + comments and neater line breaks around some Decimal(...)s, are there any other significant changes?

@WarrenWeckesser
Copy link
Member Author

WarrenWeckesser commented Sep 30, 2019

Thanks Kai.

The numpy repository doesn't have an AUTHORS.txt either, so I don't think we need it.

The essential changes in test_financial.py fix the imports to use the numpy_financial library. The rest of the changes are PEP 8 whitespace clean up. In hindsight, those could have been made in separate commits, but when I started I didn't think the PEP 8 changes would be so significant.

@Kai-Striega
Copy link
Member

That all sounds good. Do you want to keep this PR open and add CI/Docs/etc? If not, I'll run the tests when I get home tonight and confirm everything works as expected.

@WarrenWeckesser
Copy link
Member Author

I'm fine with getting the package into shape through a series of pull requests, rather than trying to do it all in this one. So separate PRs for docs and for CI, for example, would be fine. I expect all the changes will be pretty straightforward, but I don't know the details of all the machinery well enough to get it all right in one PR. I'll make an issue with a "to do" list.

@Kai-Striega
Copy link
Member

👍 I don't have write access to numpy, so I can't merge your PRs. I can do some work on CI/Docs this afternoon if you prefer to focus on something else.

@charris
Copy link
Member

charris commented Sep 30, 2019

Should we add @Kai-Striega to the numpy-financial maintainers?

@Kai-Striega
Copy link
Member

@WarrenWeckesser I've run the tests and they all pass on my machine and they all pass. As a follow-up question, in setup.py we're using NumPy's distutils, do we still need these or can we change to using only setuptools?

@charris I originally intended to only help set up / maintain as I'm concerned about being the main maintainer. Saying that, once set up, I suspect there won't be too much maintainance burden. I'll still be active within SciPy (and NumPy) - so the required maintanence should not be much more than I already intend to do.

@WarrenWeckesser WarrenWeckesser changed the title WIP: Create the basic structure of the numpy_financial package. MAINT: Create the basic structure of the numpy_financial package. Oct 1, 2019
@WarrenWeckesser
Copy link
Member Author

I'm going to self-merge this, and I'll probably continue to be quick to merge changes until we have this in good enough shape to call it pre-release.

@WarrenWeckesser WarrenWeckesser merged commit f39ffa8 into numpy:master Oct 1, 2019
@WarrenWeckesser WarrenWeckesser deleted the getnumpysource branch October 1, 2019 22:44
@WarrenWeckesser
Copy link
Member Author

@charris asked

Should we add @Kai-Striega to the numpy-financial maintainers?

Sure!

Kai, helping out now is not a commitment to become the permanent maintainer of the package. You can drop your involvement with the package at any time. Any help you can provide now during this transition phase is certainly appreciated!

@WarrenWeckesser
Copy link
Member Author

I've started self-merging PRs to get the package set up reasonably quickly.

@WarrenWeckesser WarrenWeckesser mentioned this pull request Oct 2, 2019
6 tasks
@charris
Copy link
Member

charris commented Oct 2, 2019

I've started self-merging PRs to get the package set up reasonably quickly.

That's fine. At some point the initial setup will be done and we can review the whole thing.

@Kai-Striega
Copy link
Member

@charris @WarrenWeckesser. I'd be happy to be a maintainer. I've been working on this too, I'll try to get a few PRs in over the weekend.

@WarrenWeckesser
Copy link
Member Author

Thanks, Kai. As you can see, this PR has been merged. I have a github workflow set up to run unit tests on PRs and on any push to the master branch (https://github.com/numpy/numpy-financial/blob/master/.github/workflows/pythonpackage.yml). I've been trying to get another workflow set up to build the docs and push the result to the repo's gh-pages branch, which will be rendered at https://numpy.org/numpy-financial/. What is there now is the result of a manual push of the docs. I've been trying to get an automated workflow set up, but I'm still figuring out what's possible. I am currently stuck at pushing the newly built docs back to the repo after they are built in a workflow. There are authentication issues to be worked out.

@Kai-Striega
Copy link
Member

I'll take a look at the authentication issues when I get home from work. Also, I could have been clearer, by "working on this" I meant numpy-financial at large, not this particular PR.

@WarrenWeckesser
Copy link
Member Author

Yeah, that's what I understood. 😄 This was just a convenient place to give an update.

@charris
Copy link
Member

charris commented Oct 4, 2019

@Kai-Striega I sent you an invite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.