Skip to content

Fix continuous peth nbins - #678

Open
lilytong0919 wants to merge 9 commits into
pynapple-org:devfrom
lilytong0919:fix-continuous-peth-nbins
Open

lilytong0919 wants to merge 9 commits into
pynapple-org:devfrom
lilytong0919:fix-continuous-peth-nbins

Conversation

@lilytong0919

Copy link
Copy Markdown
Contributor

Thank you for opening this PR. Filling out this template will help the maintainers of pynapple to review your contribution.

This PR template has 2 sections. Please fill the first section yourself. Optionally, description of the PR by a LLM can be included in the second section.

Contributor statement

AI Assistance Disclosure

  • No AI tools were used in preparing this PR.
  • If AI tools were used, I have disclosed which ones. I have fully reviewed, understood and verified their output.

Uses ChatGPT6 for test case generation (and check that the desired behavior is achived by the current modification

Motivation

In issue #671 I mentioned that compute_perievent give unstable number of bins on continuous data given the same window and timeseries with persumability the same bin_size. I suspected that is due to floating point percision issue when accessing the edge cases.

PR description

This PR address the issue by changing the time_idx and windowsize computation to go through checks with tolerance, and snap time_idx on the edges when the variation in time_idx is close enought to defined window. The former change allow intial creation of new_data_array that are consistent in number of bins. The latter prevent the object intializer to crop the data array on either end due to time_idx edge bins falling slightly over the defined new_time_support.

Outstanding questions / particular feedback

I moved the definition of tolerance into compute_perievent and use it as input to both _align_regular and _is_regularly_sampled. I think this ensures the assertion to regular sampling to fail before the program start giving inconsistent behavior on binsize. I am not very certain this does what I intended it to do. But for this I check with a test with random jittering (which is not a test case because this relys on random generation of data which is not so stable). The setting here should be jittering around the boundary cases of regularity check. So we would expect the print statement in the except block to be triggered multiple times, but the final comparison of bin_count only give 1 consistent value.

import pynapple as nap

rng = np.random.default_rng(0)

nominal_dt = 0.05
window = (-4, 4)
event_time = 10.0

bin_count = []

for i in range(100):
    # print("="*80)
    # print(f"Iteration {i+1}:")
    # One tiny perturbation to dt for the whole regularly sampled series
    noise_level = 1.6e-8
    t = np.arange(0, 20, nominal_dt)
    dt_error = rng.uniform(-noise_level, noise_level, size = (len(t),))

    t_error = t + dt_error
    dt = np.median(np.diff(t_error))

    tsd = nap.Tsd(
        t=t_error,
        d=np.sin(t_error),
    )

    events = nap.Ts(t=[event_time])
    try:
        peth = nap.compute_perievent(
            data=tsd,
            events=events,
            window=window,
        )
    except Exception as e:
        print(f"Error in iteration {i+1}: {e}")
        continue
    peth_binsize = peth.t[1] - peth.t[0]
    peth_times = peth.times()
    bin_count.append(len(peth.t))

    # print(f"Peth size = {peth.shape}, peth binsize = {peth_binsize:.10f}")
    # print(f"Peth start = {peth_times[0]}, peth end = {peth_times[-1]}")
print(f"All bin counts equal: {len(set(bin_count)) == 1}, Bin count: {bin_count[0]}")

Checklist

Affirm that you have done the following:

  • I have described the changes in this PR, following the template above.
  • I have added any necessary tests.
  • I have added any necessary documentation. This includes docstrings, updates to existing files found in doc/, or (for large changes) adding new files to the doc/ folder.
    I only added a small nested function _full_bin_count and several if statement. They are properly commented but no docstring is added. Since this PR only meant to make compute_perievent behave more stably, I think no need to change it's documentation.

AI description of the PR (optional)

…event

number of bins consistent until the new data array is created.

The final output actually still have the same issue with cutout boundaries. Suspect that is from the `time_support=new_time_support` argument in the return statement.
Just take the median here to get a reasonable dt.
Several reasons:
1. Rounding to 9th decimal don't actually eliminate the problem with edge cases (probably also need to be consistent with the `_is_reuglarly_sampled` tolerance value for that to work.
2. `_full_bin_count` will determine `windowsize` with tolerance with a safer logic, so rounding prior to that is unnecessary.

But I will need some fix after nbins is determined to makesure the edge bins can fall within the `new_time_support`.
Moved `_full_bin_count` to be an nested function in `_align_regular` since this is probably going to be the only use case.
Added tolerance as an argument at the level of `_align_regular`, because this needed for determining number of bins and for snapping edge bins to fit within the new timesupport.
Added tolerance as a variable that will be input to `utils._is_regularly_sampled` and `_align_regular` because they need to be consistent.
Added 3 tests for edge cases of checking regular sampling and checking consistency in number of bin counts.
@lilytong0919
lilytong0919 marked this pull request as ready for review October 20, 2026 16:28
@lilytong0919
lilytong0919 requested a review from gviejo as a code owner October 20, 2026 16:28
Copilot AI balanced review requested due to automatic review settings October 20, 2026 16:28

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

This branch has not been deployed

No deployments
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