8000 Add unit test for IntervalList creation from close ended intervals by tskisner · Pull Request #823 · hpc4cmb/toast · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add unit test for IntervalList creation from close ended intervals #823

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
May 8, 2025
Merged

Add unit test for IntervalList creation from close ended intervals #823

merged 3 commits into from
May 8, 2025

Conversation

tskisner
Copy link
Member
@tskisner tskisner commented May 6, 2025

This unit test fails, but I think it should work, based on the comments in the recently updated code. @keskitalo can you take a look and see what you think?

@tskisner tskisner requested a review from keskitalo May 6, 2025 13:55
@keskitalo
Copy link
Member

I think there is a misunderstanding here. Both time and sample definitions are open-ended, except for the very last sample time.

Say that there is an interval boundary at time T and sample S. Regardless of which definition you use, sample S will belong to the interval that starts at T, not to the one that ends at T.

In your unit test, the time-based intervals should be constructed from

timespans = [(stamps[x[0]], stamps[min(x[1], stamps.size - 1)]) for x in samplespans]

not as

timespans = [(stamps[x[0]], stamps[x[1] - 1]) for x in samplespans]

I realize that taking the minimum looks clunky, but if the user already has the sample indices, then there hardly is a reason to construct time spans by sampling the stamps vector.

@tskisner
Copy link
Member Author
tskisner commented May 6, 2025

Ok, thanks for clarifying the intended design. Creating observation intervals from timestamps is a common pattern. It is used in the spt3g interfaces, hdf5 data loading, building azimuth intervals, etc. The spt3g code is currently broken. I just fixed this unit test to capture the intended behavior and will fix the spt3g code and any other problems in a separate PR.

keskitalo
keskitalo approved these changes May 7, 2025
View reviewed changes
Copy link
Member
@keskitalo keskitalo left a comment

Choose a reason for hiding this comment

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

I would change the comment but the test is fine.

src/toast/tests/intervals.py Outdated
samplespans = [(x * n_intr, x * n_intr + n_intr) for x in range(n_intr)]
intr_samples = IntervalList(stamps, samplespans=samplespans)

# Time ranges are open ended *unless* they fall exactly on a sample.
Copy link
Member

Choose a reason for hiding this comment

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

time ranges are open-ended, unless the end time coincides with the observation end time.

@tskisner tskisner merged commit b376240 into toast3 May 8, 2025
7 checks passed
@tskisner tskisner deleted the interval_times branch May 8, 2025 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keskitalo keskitalo keskitalo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tskisner @keskitalo
0