Skip to content

Conversation

@MashyBasker
Copy link
Contributor

The --doctest-modules option of pytest is added to the original poetry run pytest for testing the documentation code examples.

@MashyBasker MashyBasker changed the title Add automated documentation testing when building. Fixes #73 Add automated documentation testing when building Dec 6, 2023
@Kai-Striega
Copy link
Member

This looks good from your side (except the tests failing) would you be able to look at those within this PR?

@MashyBasker
Copy link
Contributor Author

MashyBasker commented Dec 9, 2023

As I commented in issue#95, we probably need to use assert_allcose instead assert_almost_equal.
I saw someone was already working on a fix. I they aren't anymore, I would be happy to work on it.

This was taking too long and needs to be reduced
This commit introduces a native hot path using numba.
This leads to an ~50x speed up on the `time_broadcast`
benchmark with 100x100x100 dimension
@MashyBasker
Copy link
Contributor Author

I just tried changing every assert_almost_equal to assert_allclose. But this breaks the other tests. I'll look into it more

This leads to the following improvements on the `time_broadcast` benchmark
with dimension 100x100x100:

* raw Python: 197 ms
* numba only: 4.08 ms
* numba + prange 0.3 ms
@Kai-Striega
Copy link
Member

I don't think that's the issue (here) I think it's just that we're comparing floating point numbers in the doctests which aren't exactly equal. Could you try rounding the output of the examples to see if that fixes it?

Kai-Striega and others added 8 commits December 9, 2023 14:02
The newest released numba version is 0.58.1. This version does not support Python 3.12.
Since we are using it we also cannot support Python 3.12. This should be added once
numba version 0.59 is released.
These were originally added when the code was written as one function without numba. As numba
is now being used, and the functions are therefore seperated. This is no longer required and
makes the code more complex.
This fixes a bug where we weren't creating decimal arrays in the benchmark.
Instead of creating an array and setting the dtype we manually make a list of
Decimals and create an array from that list.
@MashyBasker
Copy link
Contributor Author

MashyBasker commented Dec 9, 2023

I have fixed the doctests and they now work fine in my local machine and my forked repository. The problems were mostly:

  • rounding error
  • # may vary comments in the output(should be provided in the function
  • Empty lines in the output that should have <BLANKLINE> command.
  • Incorrect raise exception output.

This PR resolves #95 and #73

@MashyBasker
Copy link
Contributor Author

@Kai-Striega I have fixed the linter problem in the mirr doctests

@Kai-Striega
Copy link
Member

@MashyBasker thanks. It looks like some tests are still failing.

FYI you can run the style checks yourself on your local machine. See these lines:

poetry run ruff version
# Check the source file, ignore type annotations (ANN) for now.
poetry run ruff check numpy_financial/ --ignore F403 --select E,F,B,I
# Check the test and benchmark files
poetry run ruff check tests/ benchmarks/ --select E,F,B,I

@MashyBasker
Copy link
Contributor Author

@Kai-Striega I made the changes and ruff works in my local machine perfectly too. Awaiting your feedback

@Kai-Striega
Copy link
Member

Kai-Striega commented Dec 10, 2023

There's still one line that's too long numpy_financial/_financial.py:1071:89: E501 Line too long (91 > 88) I'm not quite sure why your ruff installation didn't pick that up :(.

Otherwise I think this looks excellent and ready to go!

Sorry that there's so much fussing around with the style guide. But I'd like to keep the project consistent.

@MashyBasker
Copy link
Contributor Author

MashyBasker commented Dec 10, 2023

@Kai-Striega I've addressed the line length issue in . Apologies for any oversight, and I appreciate your diligence in maintaining the style guide :)

@Kai-Striega
Copy link
Member

Kai-Striega commented Dec 10, 2023

@MashyBasker lint is green. Give me a few minutes to look through it again, but I think this should be good to go.

For future reference could you please follow the guide for how to write the commit message? I haven't documented it for NumPy-Financial (yet) so don't feel too bad about this PR.

@MashyBasker
Copy link
Contributor Author

Thanks. I'll use the guide next time

@Kai-Striega Kai-Striega merged commit 1656639 into numpy:main Dec 10, 2023
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.

2 participants