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

feat (WIP): Mean age of childbearing #61

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

spencerpease
Copy link
Member

@spencerpease spencerpease commented Sep 1, 2020

Describe changes

Adds a function macb_from_nfx(), which calculates mean age of childbearing.

What issues are related

Fixes #60

Related to #19

Checklist

Packages Repositories

  • Have you read the contributing guidelines for ihmeuw-demographics R packages?
  • Have you successfully run devtools::check() locally?
  • Have you updated or added function (and vignette if applicable) documentation? Did you update the 'man' and 'NAMESPACE' files with devtools::document()?
  • Have you added in tests for the changes included in the PR?
  • Do the changes follow the ihmeuw-demographics code style?
  • Do your changes need to be immediately included in a new build of docker-base or docker-internal? If so follow directions in those repositories to rebuild and redeploy the images.

Other Repositories

  • Did you update any relevant documentation (README.md, script headers, wiki pages, internal IHME hub pages etc.)?
  • Could someone else on the ihmeuw-demographics team replicate or use your work using available documentation?
  • Did you test your work? Either via automated tests or documented manual testing?

Details of PR

Include other information helpful for reviewers.

  • Did you reference other sources in order to make the changes in the PR? Link those here or include in actual documentation as part of the PR changes.

This defines the API and core calculation for calculating MACB
from an input `data.table`.

References #60, #19
@spencerpease spencerpease added enhancement New feature or request good first issue Good for newcomers labels Sep 1, 2020
@spencerpease spencerpease changed the title FEAT: MACB function (WIP) feat (WIP): MACB function Sep 1, 2020
@spencerpease spencerpease changed the title feat (WIP): MACB function feat (WIP): Mean age of childbearing function Sep 1, 2020
@spencerpease spencerpease changed the title feat (WIP): Mean age of childbearing function feat (WIP): Mean age of childbearing Sep 1, 2020
@chacalle
Copy link
Collaborator

chacalle commented Sep 8, 2020

@chacalle
Copy link
Collaborator

chacalle commented Sep 8, 2020

#' @param dt \[`data.table()`\]\cr A data.table with columns 'age_start',
#' 'age_end', and a column of fertility rates named after `nfx_col`.
#' @param nfx_col \[`character(1)`\]\cr Name of fertility rate column in `dt`.
#' Defualts to "nfx".
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, we should use standard notation nfx in other functions instead of asfr.

#' @param nfx_col \[`character(1)`\]\cr Name of fertility rate column in `dt`.
#' Defualts to "nfx".
#' @inheritParams gen_lifetable_parameters
#' @param value_col \[`character(1)`\]\cr Name of output 'MACB' column. Defaults
Copy link
Collaborator

Choose a reason for hiding this comment

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

When would you not just want this to be mcab? We don't have this option in other functions.

#'
#' @export
#'
#' @examples
Copy link
Collaborator

Choose a reason for hiding this comment

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

TODO: add example(s)

assertthat::is.string(nfx_col),
assertthat::is.string(value_col)
)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Add more checks for dt and id_cols.


macb_dt <- dt[
,
.(macb = weighted.mean((age_end + age_start) / 2, get(nfx_col))),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like to use list instead of . in the packages. Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Where does the weighted.mean function come from?

#'
#' @return \[`data.table()`\] A data.table with calculated 'MACB' for each
#' unique grouping in `id_cols`, stored in `value_col`.
#'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add @details section with equation?

#' @param dt \[`data.table()`\]\cr A data.table with columns 'age_start',
#' 'age_end', and a column of fertility rates named after `nfx_col`.
#' @param nfx_col \[`character(1)`\]\cr Name of fertility rate column in `dt`.
#' Defualts to "nfx".

Choose a reason for hiding this comment

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

Spelling error

#' @inheritParams gen_lifetable_parameters
#' @param value_col \[`character(1)`\]\cr Name of output 'MACB' column. Defaults
#' to "mcab"
#'

Choose a reason for hiding this comment

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

could add @seealso for where to find more information on MACB

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FEATURE: Mean age of childbearing (MACB)
4 participants