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 347: Add enw_one_hot_encode_feature helper + example using parametric and nonparametric reporting together. #348

Merged
merged 13 commits into from Nov 7, 2023

Conversation

seabbs
Copy link
Collaborator

@seabbs seabbs commented Oct 27, 2023

Description

This PR closes #347.

  • Added a new helper function enw_one_hot_encode_feature() for one hot encoding variables and binding them into the original data. This is useful when users want to include parts of variables in their models as binary indicators - for example giving a specific delay its own effect.
  • Added a new example showcasing how to fit a model to data reported weekly with a 3 day delay until any reports are non-zero with a weekly process model and a mixture of a parametric and non-parametric reference date model.

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.

@github-actions

This comment was marked as outdated.

@seabbs seabbs marked this pull request as ready for review October 27, 2023 14:31
@codecov
Copy link

codecov bot commented Oct 27, 2023

Codecov Report

Merging #348 (bcd9244) into main (8eff560) will decrease coverage by 0.10%.
Report is 8 commits behind head on main.
The diff coverage is 85.71%.

❗ Current head bcd9244 differs from pull request most recent head 18493d1. Consider uploading reports for the commit 18493d1 to get more accurate results

@@            Coverage Diff             @@
##             main     #348      +/-   ##
==========================================
- Coverage   96.93%   96.84%   -0.10%     
==========================================
  Files          15       15              
  Lines        1862     1871       +9     
==========================================
+ Hits         1805     1812       +7     
- Misses         57       59       +2     
Files Coverage Δ
R/preprocess.R 95.67% <100.00%> (+0.01%) ⬆️
R/model-design-tools.R 97.05% <84.61%> (-2.95%) ⬇️

@seabbs seabbs self-assigned this Oct 27, 2023
@seabbs seabbs changed the title Issue 347: Add enw_hot_encode_and_bind helper + example using parametric and nonparametric reporting together. Issue 347: Add enw_one_hot_encode_feature helper + example using parametric and nonparametric reporting together. Nov 7, 2023
Copy link
Collaborator Author

@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.

[Self-review] This PR adds single example that showcases how to mix the parametric and non-parametric models. It also adds a feature and a helper function that is useful when building these models (I was surprised this helper wasn't more readily available in other packages tbh).

Given its low impact on the rest of the code base I am minded to merge this now as all looks as expected and then resolve any potentially issues prior to a new release?

@seabbs seabbs mentioned this pull request Nov 7, 2023
Copy link
Collaborator Author

@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.

[SELF-REVIEW] Final checks prior to merging.

@seabbs seabbs merged commit 1a95550 into main Nov 7, 2023
9 checks passed
@seabbs seabbs deleted the issue347 branch November 7, 2023 20:59
Copy link

github-actions bot commented Nov 7, 2023

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

  •   :ballot_box_with_check:day_of_week_model: 26.6s -> 25.9s [-5.42%, +0.13%]
  •   :ballot_box_with_check:latent_renewal_model: 30.8s -> 30.2s [-13.18%, +9.34%]
  •   :ballot_box_with_check:missingness_model: 1.32m -> 1.32m [-1.61%, +2%]
  •   :ballot_box_with_check:multi_group_latent_renewal_model: 8.07s -> 7.55s [-21.26%, +8.36%]
  •   :ballot_box_with_check:preprocessing: 687ms -> 687ms [-1.42%, +1.36%]
  •   :ballot_box_with_check:simple_model: 6.67s -> 6.26s [-31.26%, +19.11%]
  •   :ballot_box_with_check:simple_negbin_model_with_pp: 4.52s -> 5.01s [-6.49%, +28.32%]
    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.

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.

Add an example combining non-parametric and parametric delay models
1 participant