-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add kde
function and tests to RustPython statistics module
#6030
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 kde
function and tests to RustPython statistics module
#6030
Conversation
WalkthroughThe input validation in the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yml ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Files:
🔇 Additional comments (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
kde
function and tests to RustPython statistics modulekde
function and tests to RustPython statistics module
I don't know if the tests will pass with this. I still haven't fully figured out a fully working local development setup. I can build a rustpython binary which works as excpected, but
Any pointers would be appreciated if anyone spots this and is familiar with the message 🫡. |
Hey @jackoconnordev ty for the PR:) Does: cargo run --release -- -m test -j1 -v -u cpu test_statistics -m "TestKDE" works for you? (assuming you're checked to your branch) Also, it would be better if you could update both If any tests fails you can mark them with: # TODO: RUSTPYTHON
@unittest.expectedFailure
def test_X(self):
... |
Happy to and thanks for the quick review! I tried that test command now and got two AttributeErrors:
I'll put this back into Draft and try porting the entire files from CPython 💪. |
## test_kde I'm not too sure why but this one takes a few seconds to run the second for loop which calculates the cumulative distribution and does a rough calculation of the area under the curve. ## test_kde_random I have a lower bound for RustPython to sort a random list of 1_000_000 numbers on my laptop of > 1 hour. By dropping n to 30_000 sort will not take an egregious amount of time to run. It is then necessary to lower the tolerance for the math.isclose check, or the computed values may **randomly** fail due to the higher variance caused by the smaller sample size.
d8ff7f0
to
bdf71bd
Compare
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.
Thank you for contributing!
@ShaharNaveh Thank you so much for the review!
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.
test_inv_cdf requires investigation. Adding back @unittest.expectedFailure
will be enough for test_slots
Relevant CPython PR which introduced the failing assertion to |
See python/cpython#95265. To quote: > Restores alignment with random.gauss(mu, sigma) and random.normalvariate(mu, sigma) both. of which are equivalent to sampling from NormalDist(mu, sigma).inv_cdf(random()). The two functions in the random module happy accept sigma=0 and give a well-defined result. > This also lets the function gently handle a sigma getting smaller, eventually becoming zero. As sigma decrease, NormalDist(mu, sigma).inv_cdf(p) forms a tighter and tighter internal around mu and becoming exactly mu in the limit. For example, NormalDist(100, 1E-300).inv_cdf(0.3) cleanly evaluates to 100.0but withsigma=1e-500`` the function previously would raised an unexpected error.
I've run the full test_statistics suite and it's passing on my laptop now The pytest tests in |
I usually don't run |
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.
Thank you!
Description
Adds the
kde
function to RustPython's standard library alongside Python unit tests. Function and tests were copied as is from CPython 3.13, but in order for the new tests to complete on the order of seconds instead of hours I adjusted some "magic constants".From my local testing, the builtin
sorted
function is the bottleneck here. CPython will sort a list of 1 million random integers in a few hundred milliseconds making it suitable to be used in a unit test. RustPython is not yet capable of this.Using
kde
in the interpreterThis example is taken from
kde
function's docstring.whats_left.py diff
Following is the result of diffing the output of
python -I whats_left.py
with and without the kde function added.Example output showing original numbers are too slow to be appropriate
Summary by CodeRabbit