Skip to content

Fix unstable output of periodic sequence due to precision error. - #35311

Merged
shunping merged 2 commits into
apache:masterfrom
shunping:unstable-periodic-sequence
Jun 17, 2025
Merged

Fix unstable output of periodic sequence due to precision error.#35311
shunping merged 2 commits into
apache:masterfrom
shunping:unstable-periodic-sequence

Conversation

@shunping

@shunpingshunping commented Jun 16, 2025

Copy link
Copy Markdown
Collaborator

A fix following #35300.

See the explanation below (copied from the inline comments):

In ImpulseSeqGenRestrictionProvider, the total number of counts (i.e. total_outputs) is computed by ceil((end - start) / interval) (

total_outputs=math.ceil((end-start) /interval)
), where end is start + duration (
stop=start+min(self._duration, self._max_duration)
).

Due to precision error of arithmetic operations, even if duration is set to len(self._data) * interval, (end - start) / interval could be a little bit smaller or bigger than len(self._data).

In case of being bigger, total_outputs would be len(self._data) + 1, as the ceil() operation is used, which is NOT what we want.

Assuming that the precision error is no bigger than 1%, by subtracting a small amount, we ensure that the result after ceil() is stable even if a certain degree of precision error is present.

@shunpingshunping self-assigned this Jun 16, 2025
@shunping
shunpingforce-pushed the unstable-periodic-sequence branch from 7bcd8d4 to 3d15fa0CompareJune 16, 2025 20:16
@shunping
shunping marked this pull request as ready for review June 16, 2025 23:39
@shunping
shunping requested a review from claudevdmJune 16, 2025 23:40
@shunping

Copy link
Copy Markdown
CollaboratorAuthor

@claudevdm, could you please review this one too since you did the previous one and have some context? Thanks a lot!

@github-actions

Copy link
Copy Markdown
Contributor

Assigning reviewers:

R: @jrmccluskey for label python.

Note: If you would like to opt out of this review, comment assign to next reviewer.

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

@shunping

Copy link
Copy Markdown
CollaboratorAuthor

stop reviewer notifications

@shunping

Copy link
Copy Markdown
CollaboratorAuthor

remind me after tests pass

@github-actions

Copy link
Copy Markdown
Contributor

Stopping reviewer notifications for this pull request: requested by reviewer. If you'd like to restart, comment assign set of reviewers

@shunping

Copy link
Copy Markdown
CollaboratorAuthor

Thanks @claudevdm!

@shunping
shunping merged commit 2cd8d07 into apache:masterJun 17, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@shunping@claudevdm