-
Notifications
You must be signed in to change notification settings - Fork 37
allow snapshots shorter than trunc_max #438
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.
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
Conversation
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 9493405 is merged into main:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good. I would probably phrase this as a new feature vs fixing a bug as the function was never meant to work when snapshots < delays (though this was of course not communicated).
Just needs a dev version bump.
63b6fd5
to
bff2f15
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
bff2f15
to
8bcd8bb
Compare
Done in 4bfc01e |
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 195a1c4 is merged into main:
|
Ruh Roh |
2ac0e7e
to
d81e94f
Compare
This is how benchmark results would change (along with a 95% confidence interval in relative change) if bce1d56 is merged into main:
|
d81e94f
to
9cec97f
Compare
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 6e4c9dd is merged into main:
|
9cec97f
to
ba060db
Compare
This is how benchmark results would change (along with a 95% confidence interval in relative change) if e482384 is merged into main:
|
estimate_truncation currently fails when the any snapshot is shorter than
trunc_max
. This PR fixes that by provding explicit bounds and vector elements in the appropriate place.