pFad - Phone/Frame/Anonymizer/Declutterfier! Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

URL: http://github.com/astropy/astropy/pull/19378

ets/global-0bd78641c0a1f3e0.css" /> TST: Add direct tests for ks_2samp Cython extension in stats._stats by dhruv1955 · Pull Request #19378 · astropy/astropy · GitHub
Skip to content

TST: Add direct tests for ks_2samp Cython extension in stats._stats#19378

Open
dhruv1955 wants to merge 2 commits intoastropy:mainfrom
dhruv1955:tst/stats-ks2samp-cython
Open

TST: Add direct tests for ks_2samp Cython extension in stats._stats#19378
dhruv1955 wants to merge 2 commits intoastropy:mainfrom
dhruv1955:tst/stats-ks2samp-cython

Conversation

@dhruv1955
Copy link
Copy Markdown
Contributor

Summary

ks_2samp in astropy/stats/_stats.pyx is a Cython implementation of the two-sample KS statistic used internally by astropy.stats, but it currently has no tests that call it directly.

This PR adds a dedicated test file that exercises the Cython function independently of the public Python API.

What’s Tested

  • Identical arrays return 0.0
  • Completely non-overlapping arrays return 1.0
  • Partially overlapping arrays match SciPy's result
  • Different-sized arrays match SciPy's result
  • Integer dtypes (int32, uint8) work correctly
  • Single-element arrays
  • Result is always between 0.0 and 1.0

@dhruv1955 dhruv1955 requested a review from larrybradley as a code owner March 7, 2026 20:16
@github-actions github-actions bot added the stats label Mar 7, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 7, 2026

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@pllim pllim added this to the v8.0.0 milestone Mar 9, 2026
@pllim
Copy link
Copy Markdown
Member

pllim commented Mar 9, 2026

I don't think anyone asked for this as you did not link issue.

@dhruv1955
Copy link
Copy Markdown
Contributor Author

Thanks @pllim for the feedback. I'll open issues for these test additions first and link them before reopening. The motivation is the GSoC 2026 project on hardening astropy's core stability, which specifically calls for a low-level test suite for C/Cython extensions independent of the public API.

@pllim
Copy link
Copy Markdown
Member

pllim commented Mar 9, 2026

GSoC 2026 project on hardening astropy's core stability

Huh. Has GSoC started? Who is your mentor?

@dhruv1955
Copy link
Copy Markdown
Contributor Author

Huh. Has GSoC started? Who is your mentor?

GSoC 2026 applications are open until April 8th. I'm in the contributor application phase — not yet accepted. The project is listed at https://openastronomy.org/gsoc/gsoc2026/#/apply with neutrinoceros, astrofrog, and nstarkman as mentors. I'm contributing to astropy to strengthen my application.

@pllim
Copy link
Copy Markdown
Member

pllim commented Mar 9, 2026

Ah, gotcha. In that case, please clarify your motivation in your future PR descriptions and ping a mentor to review. Thanks for your interest.

cc @neutrinoceros @astrofrog @nstarman

@pllim
Copy link
Copy Markdown
Member

pllim commented Mar 9, 2026

Also, I think any maintainer can re-open this PR if they find it valuable and wish to proceed. FYI.

@neutrinoceros
Copy link
Copy Markdown
Contributor

Indeed this is in line with our GSoC proposal. I'll re-open and review now.
@pllim, please feel free to ping me on similar PRs if you see more of them.

@dhruv1955 dhruv1955 force-pushed the tst/stats-ks2samp-cython branch from bcf89b5 to 680046c Compare March 10, 2026 16:00
@dhruv1955
Copy link
Copy Markdown
Contributor Author

Thanks @neutrinoceros Addressed all feedback and squashed to a single commit:

  • Renamed file to test_ks_2samp.py
  • Renamed test functions (removed redundant suffixes)
  • Moved scipy dependency to @pytest.mark.skipif on individual tests only
  • Merged three dtype tests into one @pytest.mark.parametrize test
  • Removed np.sort() from already-sorted arrays
  • Removed changelog fragment

@dhruv1955 dhruv1955 force-pushed the tst/stats-ks2samp-cython branch 2 times, most recently from a455676 to b2b3bf5 Compare March 10, 2026 16:49
@dhruv1955
Copy link
Copy Markdown
Contributor Author

Thanks @neutrinoceros Replaced the manual try/except block with from astropy.utils.compat.optional_deps import HAS_SCIPY as suggested.

Copy link
Copy Markdown
Contributor

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

Now comes the part where I learn about the algorithm in question (ks_2samp).
These tests look like a good first step but they're not strict enough, I think.
One general observation is that it might make more sense to use pytest.approx in all of them, with explicit tolerances as low as possible.

@dhruv1955 dhruv1955 force-pushed the tst/stats-ks2samp-cython branch from b2b3bf5 to 37ff503 Compare March 11, 2026 16:30
@dhruv1955
Copy link
Copy Markdown
Contributor Author

Thanks @neutrinoceros Replaced the scipy comparisons with analytically computed expected values and pytest.approx. Removed the scipy dependency entirely since all tests now use hard-coded values. Squashed to a single commit.

@astrofrog
Copy link
Copy Markdown
Member

astrofrog commented Mar 11, 2026

In general we use assert_allclose from numpy rather than pytest.approx, so might be worth switching to that for consistency?

@dhruv1955 dhruv1955 force-pushed the tst/stats-ks2samp-cython branch from 37ff503 to 677dedf Compare March 11, 2026 19:51
@dhruv1955
Copy link
Copy Markdown
Contributor Author

Thanks @astrofrog, Switched to assert_allclose from numpy.testing throughout for consistency.

@dhruv1955
Copy link
Copy Markdown
Contributor Author

Hi @neutrinoceros - when you get a chance, would appreciate a look at the proposal. Happy to address any feedback before the deadline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

pFad - Phonifier reborn

Pfad - The Proxy pFad © 2024 Your Company Name. All rights reserved.





Check this box to remove all script contents from the fetched content.



Check this box to remove all images from the fetched content.


Check this box to remove all CSS styles from the fetched content.


Check this box to keep images inefficiently compressed and original size.

Note: This service is not intended for secure transactions such as banking, social media, email, or purchasing. Use at your own risk. We assume no liability whatsoever for broken pages.


Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy