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

change the default value of sample in dist_fit() #389

Merged
merged 7 commits into from Jun 9, 2023

Conversation

jamesmbaazam
Copy link
Contributor

@jamesmbaazam jamesmbaazam commented May 11, 2023

In the current design of dist_fit(), the argument sample is supplied with a default value of NULL, which is checked and replaced with 1000 if it's NULL or < 1000. I think that means it should have a default value of 1000 with a check to see if it's < 1000 when the default is overridden with a supplied value.

In this PR, I change the default value of samples to 1000. If the value of samples is supplied, the function checks if samples is <1000, and if so, sets it to 1000 and throws a warning to inform the user.

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.

Thanks, James,

See the note about the warning message.

This needs:

  • To be rebased on main
  • To be redoc'd
  • To have a news item
  • To have a dev version increment.

Then good to merge.

R/dist.R Outdated Show resolved Hide resolved
@github-actions
Copy link

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

  •   :ballot_box_with_check:default: 58.7s -> 58.1s [-14.16%, +12.3%]
  •   :ballot_box_with_check:no_delays: 44.4s -> 47s [-6.75%, +18.3%]
  •   :ballot_box_with_check:random_walk: 14.8s -> 15s [-7.94%, +11.35%]
  •   :ballot_box_with_check:stationary: 37.3s -> 35s [-16.11%, +3.89%]
  •   :ballot_box_with_check:uncertain: 54.4s -> 59.9s [-7.82%, +28.05%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@sbfnk
Copy link
Contributor

sbfnk commented Jun 7, 2023

Last missing task I think is

[ ] To have a dev version increment.

@jamesmbaazam
Copy link
Contributor Author

I believe this was done here 58db941.

@seabbs
Copy link
Contributor

seabbs commented Jun 7, 2023

That isn't in this PR?

@seabbs
Copy link
Contributor

seabbs commented Jun 7, 2023

Once things are ready for review FYI I need to be repinged or I won't notice!

@jamesmbaazam
Copy link
Contributor Author

Last missing task I think is

[ ] To have a dev version increment.

This has been addressed in e91568b.

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.

Nearly there. Just a small formatting issue.

NEWS.md Outdated Show resolved Hide resolved
@jamesmbaazam jamesmbaazam requested a review from seabbs June 9, 2023 09:05
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.

LGTM!

@seabbs seabbs merged commit 4abdb01 into epiforecasts:main Jun 9, 2023
9 checks passed
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.

None yet

3 participants