FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

TST: Add direct C-boundary tests for LombScargle cython_impl by ReemHamraz · Pull Request #20222 · astropy/astropy · GitHub

/ astropy Public

TST: Add direct C-boundary tests for LombScargle cython_impl - #20222

Open
ReemHamraz wants to merge 1 commit into
astropy:mainfrom
ReemHamraz:lombscargle-cython-tests
ConversationCommits (1)ChecksFiles changed
Open

TST: Add direct C-boundary tests for LombScargle cython_impl#20222
ReemHamraz wants to merge 1 commit into
astropy:mainfrom
ReemHamraz:lombscargle-cython-tests

Conversation

ReemHamraz commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Hi everyone! This PR tackles the LombScargle Cython extension testing for my GSoC project.

Instead of routing through the high-level LombScargle API, this suite hits the Cython wrapper (lombscargle_cython) directly to ensure the math and memory allocations hold up in complete isolation.

Key additions:

  • Fed strictly-typed np.float64 memory buffers directly into the C-boundary.
  • Built a SyntheticSignal fixture to mathematically verify the C-engine accurately recovers the period and power from a planted sine wave. (Note: I tuned the frequency grid to exactly 91 steps so the peak frequency lands perfectly on a discrete float point, avoiding false-negative Cython interpolation errors.)
  • Used strict type(power) is np.ndarray assertions to guarantee the boundary returns a raw array and strips any unintended subclasses.
  • Added isolated tests for the C-level dimensionality trapdoors. (Note: I deliberately passed 2D arrays that can successfully broadcast against each other to bypass NumPy's early broadcast_arrays panic, ensuring the error is actually caught by the ndim != 1 check inside the Cython wrapper.)

cc: @neutrinoceros

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

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 added this to the v8.1.0 milestone Aug 10, 2026
pllim requested a review from neutrinoceros August 10, 2026 18:59
neutrinoceros reviewed Aug 16, 2026
View reviewed changes

neutrinoceros left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

a couple simple questions and suggestions. LGTM otherwise? Thanks Reem !

Comment thread astropy/timeseries/periodograms/lombscargle/implementations/tests/test_cython_impl.py Outdated

@pytest.fixture
def perfect_sine_wave() -> SyntheticSignal:
"""Generates a mathematically perfect synthetic signal to verify period recovery."""

neutrinoceros Aug 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

"mathematically perfect" is ill-defined, especially wherever floating points arithmetics is involved. I'd suggest narrowing down the description to make it more useful and accurate

Comment thread astropy/timeseries/periodograms/lombscargle/implementations/tests/test_cython_impl.py Outdated

assert type(power) is np.ndarray
assert power.dtype == np.float64
assert np.any(np.isfinite(power))

neutrinoceros Aug 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

this feels a bit surprising (not saying it's incorrect !) from a naive standpoint. Does the implementation offer any guarantee regarding NaNs and infinities ?
(I'm deliberately avoiding reading it so as to see if tests are enough to understand it)

ReemHamraz Aug 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yeah understandable, it actually doesn't really guarantee no NaNs if it's fed bad data (like zero variance). That line is just a low-level sanity check to prove that a valid input successfully returns real floats instead of un-initialized memory garbage. I've swapped it to np.all and added a quick inline comment too!!

Comment thread astropy/timeseries/periodograms/lombscargle/implementations/tests/test_cython_impl.py Outdated
Comment on lines +64 to +66
@dataclass(kw_only=True, slots=True, frozen=True)
class NormMatrix:
norm_type: str

neutrinoceros Aug 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

a single-member zero-method immutable dataclass is probably overkill here. I'd suggest using a NewType instead, but a raw str would seem sufficient too

Comment thread astropy/timeseries/periodograms/lombscargle/implementations/tests/test_cython_impl.py Outdated
assert np.any(np.isfinite(power))


def test_cython_lombscargle_trapdoors(perfect_sine_wave: SyntheticSignal):

neutrinoceros Aug 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

improve search

Suggested change
def test_cython_lombscargle_trapdoors(perfect_sine_wave: SyntheticSignal):
def test_cython_lombscargle_exceptions(perfect_sine_wave: SyntheticSignal):

neutrinoceros commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Note: I tuned the frequency grid to exactly 91 steps so the peak frequency lands perfectly on a discrete float point, avoiding false-negative Cython interpolation errors.)

this seems worthy of some inline comment in the fixture

ReemHamraz force-pushed the lombscargle-cython-tests branch from b1ca94c to 876a56d Compare August 16, 2026 10:45
ReemHamraz marked this pull request as ready for review August 16, 2026 11:01
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

neutrinoceros neutrinoceros left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

timeseries

Projects

None yet

Milestone

v8.1.0

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL