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

Rate limits caps for fertility, mortality, and prevalence rates #273

Merged
merged 12 commits into from
Jun 16, 2023

Conversation

celiot-IDM
Copy link
Collaborator

This is a partial BREAKING CHANGE.

  1. The code assumes there is a "ChangeRateLimits" sheet with appropriate format. (As with all sheets, the default name can be overridden.)
  2. The rate limits defined in the ChangeRateLimits sheet will limit future rate predictions, which will affect time-on-task etc calculations. This is a "partial" breaking change because if the rate limits are set far enough out, there should be no changes.

Addresses issue #119 (refer for more details)

@celiot-IDM celiot-IDM added the enhancement New feature or request label Jun 9, 2023
@celiot-IDM celiot-IDM added this to the V1.1 - PACE-HRH refresh milestone Jun 9, 2023
@celiot-IDM celiot-IDM requested a review from MeWu-IDM June 9, 2023 20:15
@celiot-IDM
Copy link
Collaborator Author

@MeWu-IDM : lots of snapshots have changed. Over to you!

@celiot-IDM
Copy link
Collaborator Author

@MeWu-IDM : Just checking that you saw that the NA change limits problem should be addressed now.

}

.applyRateLimits <- function(rates, limits) {
if (!is.null(limits$max)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

there is no checks for max being negative values so this may generate weird results when setting negative max

Copy link
Collaborator

Choose a reason for hiding this comment

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

We also need to check if limits$max> limit$min, otherwise the logic does not make sense

\item{stochasticParametersSheetName}{Name of Excel sheet holding stochastic
parameters (mean, variance, etc) for different computations}

\item{changeRateLimitsSheetName}{Name of Excel sheet holding max/min ranges
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need to update the vignetter/pacehhrh.rmd to have section to explain the ChangeRateLimit spreadsheet based ion #119 logic as they are not very straightforward

Copy link
Collaborator

Choose a reason for hiding this comment

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

Moving this to issue #246.

@github-actions
Copy link

Code Coverage

Package Line Rate Complexity Health
pacehrh 94% 0
Summary 94% (2510 / 2660) 0

Minimum allowed line rate is 60%

@celiot-IDM
Copy link
Collaborator Author

@MeWu-IDM : Rate limit validation code has been checked in, and all checks have passed. Over to you!

Copy link
Collaborator

@rhanIDM rhanIDM left a comment

Choose a reason for hiding this comment

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

The change rate limits work as expected. The documentation for implementation details and expected behavior will be addressed by issue #246.

@MeWu-IDM MeWu-IDM merged commit 29525a2 into main Jun 16, 2023
@celiot-IDM
Copy link
Collaborator Author

Thank you @MeWu-IDM and @rhanIDM !

@celiot-IDM celiot-IDM deleted the charles/issue119 branch June 19, 2023 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants