Skip to content

fix: correct fftfreq sampling rate and histogram bin edges#80

Open
Omiii-215 wants to merge 2 commits into
StingraySoftware:mainfrom
Omiii-215:fix/fourier-fftfreq
Open

fix: correct fftfreq sampling rate and histogram bin edges#80
Omiii-215 wants to merge 2 commits into
StingraySoftware:mainfrom
Omiii-215:fix/fourier-fftfreq

Conversation

@Omiii-215
Copy link
Copy Markdown

Fix two bugs in the fourier module:

  1. fftfreq(n_bin, dt) → fftfreq(n_bin, 1/dt): Julia's FFTW.fftfreq expects the sampling rate (1/dt), not the sample spacing (dt). The Python convention (numpy.fft.fftfreq) is the opposite, which caused incorrect frequency arrays when porting.

  2. fit(Histogram, ...; nbins=n_bin) → fit(Histogram, ..., edges): Using nbins does not guarantee bin edges align with segment boundaries. Explicit edges via range(0, stop=e-s, length=n_bin+1) ensure deterministic, gap-free binning of unbinned event data.

Fix two bugs in the fourier module:

1. fftfreq(n_bin, dt) → fftfreq(n_bin, 1/dt): Julia's FFTW.fftfreq
   expects the sampling rate (1/dt), not the sample spacing (dt).
   The Python convention (numpy.fft.fftfreq) is the opposite, which
   caused incorrect frequency arrays when porting.

2. fit(Histogram, ...; nbins=n_bin) → fit(Histogram, ..., edges):
   Using nbins does not guarantee bin edges align with segment
   boundaries. Explicit edges via range(0, stop=e-s, length=n_bin+1)
   ensure deterministic, gap-free binning of unbinned event data.
@fjebaker
Copy link
Copy Markdown
Collaborator

fjebaker commented Jun 2, 2026

This looks great! Could you add a test that checks the output of these two things, so that if they were ever regressed the tests fail.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 2, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.08%. Comparing base (2a7829d) to head (36efe9c).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #80      +/-   ##
==========================================
+ Coverage   88.06%   88.08%   +0.02%     
==========================================
  Files           5        5              
  Lines        1081     1083       +2     
==========================================
+ Hits          952      954       +2     
  Misses        129      129              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Omiii-215
Copy link
Copy Markdown
Author

Committed the tests @fjebaker 👍🏻

Comment thread test/test_fourier.jl Outdated
@testset "test_fftfreq_uses_sampling_rate" begin
n = 100
dt = 0.5
freq = fftfreq(n, 1/dt)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Okay, but this test is not testing the Stingray.jl code, only the FFTW library. We could undo your fix commit and this test would still pass. Make sure you're testing the Stringray.jl library and not one of the dependencies (~;

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thank you! Will be Doing that !

@Omiii-215
Copy link
Copy Markdown
Author

Now both tests properly exercise Stingray.jl code

  • fftfreq test → calls avg_pds_from_events, checks returned frequencies
  • histogram test → calls get_flux_iterable_from_segments, checks no events are dropped (sum(flux) == expected_count)

@Omiii-215 Omiii-215 force-pushed the fix/fourier-fftfreq branch from f260402 to 36efe9c Compare June 2, 2026 11:32
@Omiii-215 Omiii-215 requested a review from fjebaker June 2, 2026 11:37
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.

3 participants