Skip to content
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

Clearer distinguish length and max #468

Merged
merged 9 commits into from Oct 11, 2023
Merged

Clearer distinguish length and max #468

merged 9 commits into from Oct 11, 2023

Conversation

sbfnk
Copy link
Contributor

@sbfnk sbfnk commented Oct 4, 2023

Fixes #410

Also updates examples and tests to have their max value reduced by 1 (as the previous interpretation of max parameters was to be the length).

This required a slight change to what dist_skel to return NA if asking for the probability mass of a value that is greater than the max.

@sbfnk sbfnk force-pushed the max-is-max branch 6 times, most recently from c19b03a to 7cd702d Compare October 11, 2023 13:58
@epiforecasts epiforecasts deleted a comment from github-actions bot Oct 11, 2023
@epiforecasts epiforecasts deleted a comment from github-actions bot Oct 11, 2023
@epiforecasts epiforecasts deleted a comment from github-actions bot Oct 11, 2023
@epiforecasts epiforecasts deleted a comment from github-actions bot Oct 11, 2023
@github-actions
Copy link

This is how benchmark results would change (along with a 95% confidence interval in relative change) if b9de896 is merged into main:

  •   :ballot_box_with_check:default: 44.4s -> 45.9s [-9.42%, +16.03%]
  •   :ballot_box_with_check:no_delays: 45.1s -> 47.6s [-5.9%, +16.64%]
  •   :ballot_box_with_check:random_walk: 14.1s -> 15s [-1.93%, +15.11%]
  •   :ballot_box_with_check:stationary: 27.8s -> 27.9s [-6.59%, +7.83%]
  •   :ballot_box_with_check:uncertain: 1.11m -> 1.16m [-7.13%, +17.66%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@sbfnk
Copy link
Contributor Author

sbfnk commented Oct 11, 2023

LGTM (self-review).

@seabbs would you like to review or happy for it to be merged as is?

Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

All looks good to me.

@sbfnk sbfnk merged commit 08eee77 into main Oct 11, 2023
@sbfnk sbfnk deleted the max-is-max branch October 11, 2023 18:19
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.

Distribution max is really the length
3 participants