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

Issue 344: Rename enw_delay_filter() #365

Merged
merged 11 commits into from Nov 17, 2023

Conversation

kathsherratt
Copy link
Collaborator

@kathsherratt kathsherratt commented Nov 15, 2023

Description

This PR closes #344.

  • Renames enw_delay_filter() to enw_filter_delay()
  • Deprecated enw_delay_filter() with hard stop error (though this might be too harsh... I later noticed previous epinowcast depreciations used deprecate_warn() )

Checklist

  • My PR is based on a package issue and I have explicitly linked it.
  • I have included the target issue or issues in the PR title in the for Issue(s) issue-numbers: PR title
  • I have read the contribution guidelines.
  • I have tested my changes locally.
  • I have added or updated unit tests where necessary.
  • I have updated the documentation if required.
  • My code follows the established coding standards.
  • I have added a news item linked to this PR.
  • I have updated the package development version by one increment in both NEWS.md and the DESCRIPTION.
  • I have reviewed CI checks for this PR and addressed them as far as I am able.

This introduces a hard stop so calling the (old) function name fails and gives error message:

```
Error:
! `enw_delay_filter()` was
  deprecated in <NA> 0.2.2 and is now
  defunct.
ℹ Please use `enw_filter_delay()` instead.
ℹ Please file an issue if deprecating this
  function has caused any issues.
```

I set "when" to the current version release number (0.2.2) but not sure what correct practice is for this.
Automated updates from running devtools::check()
realised this should prob be the current version, 0.2.3!
@kathsherratt kathsherratt changed the title Issue #344: Rename enw_delay_filter() Issue 344: Rename enw_delay_filter() Nov 15, 2023
Copy link

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

  •   :ballot_box_with_check:day_of_week_model: 44.9s -> 46.7s [-2.8%, +10.68%]
  •   :ballot_box_with_check:latent_renewal_model: 49.4s -> 50.6s [-9.54%, +14.6%]
  •   :ballot_box_with_check:missingness_model: 2.3m -> 2.39m [-0.61%, +8.66%]
  •   :ballot_box_with_check:multi_group_latent_renewal_model: 12.9s -> 14.9s [-8.09%, +37.8%]
  •   :ballot_box_with_check:preprocessing: 1.24s -> 1.23s [-4.75%, +3.31%]
  •   :ballot_box_with_check:simple_model: 11.1s -> 10.2s [-60.56%, +43.9%]
  •   :ballot_box_with_check:simple_negbin_model_with_pp: 7.19s -> 8.38s [-12.02%, +44.91%]
    These benchmarks are based on package examples which are available here. Further explanation regarding interpretation and methodology can be found in the documentation of touchstone.

Copy link
Collaborator

@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 @kathsherratt! This is great. I think we probably want to stick with deprecate_warn here unless you think strongly?

Once you've had a chance to think about my comments could you ping me for another review?

Could you also add yourself to the package DESCRIPTION as a contributor?

NEWS.md Outdated Show resolved Hide resolved
R/preprocess.R Show resolved Hide resolved
R/preprocess.R Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d4a65aa) 96.85% compared to head (11ae6f2) 96.85%.

❗ Current head 11ae6f2 differs from pull request most recent head aebec3a. Consider uploading reports for the commit aebec3a to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #365   +/-   ##
=======================================
  Coverage   96.85%   96.85%           
=======================================
  Files          15       15           
  Lines        1875     1875           
=======================================
  Hits         1816     1816           
  Misses         59       59           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

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

  •   :ballot_box_with_check:day_of_week_model: 26.2s -> 26.9s [-1.21%, +6.87%]
  •   :ballot_box_with_check:latent_renewal_model: 30.7s -> 29.9s [-14.5%, +9.11%]
  •   :ballot_box_with_check:missingness_model: 1.35m -> 1.32m [-5.63%, +2.07%]
  •   :ballot_box_with_check:multi_group_latent_renewal_model: 7.66s -> 7.75s [-9.66%, +12.06%]
  •   :ballot_box_with_check:preprocessing: 711ms -> 708ms [-2.08%, +1.3%]
  •   :ballot_box_with_check:simple_model: 6.57s -> 8.01s [-42.63%, +86.57%]
  •   :ballot_box_with_check:simple_negbin_model_with_pp: 4.83s -> 5.87s [-18.75%, +61.37%]
    These benchmarks are based on package examples which are available here. Further explanation regarding interpretation and methodology can be found in the documentation of touchstone.

@kathsherratt
Copy link
Collaborator Author

@seabbs pinging for a second review

Copy link

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

  •   :ballot_box_with_check:day_of_week_model: 25.9s -> 26.5s [-1.17%, +5.65%]
  •   :ballot_box_with_check:latent_renewal_model: 30.6s -> 28.8s [-21.68%, +9.75%]
  •   :ballot_box_with_check:missingness_model: 1.34m -> 1.34m [-3.25%, +3.93%]
  •   :ballot_box_with_check:multi_group_latent_renewal_model: 7.75s -> 7.33s [-14.47%, +3.67%]
  •   :ballot_box_with_check:preprocessing: 705ms -> 702ms [-1.62%, +0.71%]
  •   :ballot_box_with_check:simple_model: 5.84s -> 5.97s [-35.2%, +39.68%]
  •   :ballot_box_with_check:simple_negbin_model_with_pp: 6.52s -> 4.89s [-99.49%, +49.35%]
    These benchmarks are based on package examples which are available here. Further explanation regarding interpretation and methodology can be found in the documentation of touchstone.

Copy link
Collaborator

@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. Thanks a lot Kath!

@seabbs seabbs added this pull request to the merge queue Nov 16, 2023
Merged via the queue into epinowcast:main with commit ef6c955 Nov 17, 2023
10 checks passed
NEWS.md Show resolved Hide resolved
Copy link
Collaborator

@jamesmbaazam jamesmbaazam left a comment

Choose a reason for hiding this comment

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

Just noticed a minor mistake in the news item.

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.

enw_filter_delay function: Filter based on report - reference date
3 participants