Skip to content

Conversation

@hiijoshi
Copy link

@hiijoshi hiijoshi commented Apr 7, 2025

Overview
This PR improves the robustness of sampling rate extraction from SNIRF files by replacing a potentially unsafe .round() operation with a more reliable np.allclose() check. This change ensures compatibility with high-precision data and avoids unintended misclassification of uniformly-sampled data as jittered.

Problem
In _extract_sampling_rate(), the existing implementation uses:

python
Copy
Edit
uniq_periods = np.unique(periods.round(decimals=4))
This approach may incorrectly group near-equal values due to rounding, leading to false positives when detecting jitter.

Solution
This PR:

Replaces the rounding + uniqueness check with np.allclose(periods, mean_period, rtol=1e-6)

Retains jitter detection logic with improved precision

Keeps the warning message for users when jitter exceeds 1% threshold

Benefits
Better support for high-resolution SNIRF datasets

More accurate sampling rate detection

Fewer false warnings about jitter

Cleaner code and better alignment with scientific precision standards

🔬 Test Strategy
Although the function is part of a data I/O pipeline, the following unit tests are proposed:

✅ New Tests:
Test Perfectly Uniform Sampling

Input: Linearly spaced time_data

Expected: No warning, correct sampling rate

Test Sampling With Very Small Precision Jitter

Input: time_data with tiny noise added (below 1% jitter)

Expected: Sampling rate computed, info logged, no warning raised

Test Clearly Jittered Data

Input: time_data with jitter > 1%

Expected: Sampling rate computed, warning raised

✅ Example Test Snippet (using pytest + mne.utils._logging.catch_logging):
python
Copy
Edit
import numpy as np
from mne.io import read_raw_snirf
from mne.utils._logging import catch_logging

def test_uniform_sampling_rate_extraction():
class DummyDat:
def get(self, path):
if path == "nirs/data1/time":
return np.linspace(0, 10, 1001)

sr = _extract_sampling_rate(DummyDat())
assert np.isclose(sr, 100.0), "Expected sampling rate 100 Hz"

def test_jitter_warning(caplog):
class DummyDat:
def get(self, path):
if path == "nirs/data1/time":
t = np.linspace(0, 10, 1001)
t[500] += 0.02 # Introduce artificial jitter
return t

with caplog.at_level("WARNING"):
    _extract_sampling_rate(DummyDat())
    assert "Found jitter" in caplog.text

…cision-Aware Check

Improve SNIRF Sampling Rate Estimation by Replacing Rounding with Precision-Aware Check
@hiijoshi hiijoshi requested a review from rob-luke as a code owner April 7, 2025 15:30
@welcome
Copy link

welcome bot commented Apr 7, 2025

Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴

@drammock
Copy link
Member

drammock commented Apr 7, 2025

duplicate of #13184

@drammock drammock closed this Apr 7, 2025
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