Skip to content

epiprocess doc cleanup #563

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.

Sign up for GitHub

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

Jump to bottom
Open
14 tasks done
brookslogan opened this issue Nov 4, 2024 · 3 comments · May be fixed by #625
Open
14 tasks done

epiprocess doc cleanup #563

brookslogan opened this issue Nov 4, 2024 · 3 comments · May be fixed by #625
Assignees
@brookslogan

Comments

@brookslogan
Copy link
Contributor

brookslogan commented Nov 4, 2024 •
edited
Loading

  • Fix non-linked vignette("backtesting", package="epipredict") in archive vignette; downlit should be linking this if it exists? downlit might be unable to link because we're not on CRAN yet.
  • "Data frame returns will be tidyr::unpack()-ed, if named, and will be tidyr::pack-ed columns, if not named."
    • This is the opposite of what happens; we pack only if they are "named" using .new_col_name =. Readers may also confuse "named" with "has column names", which data.frames should/must have, or maybe even row names.
  • Link/use to deaths-from-cases&deaths forecasting example instead of current bad examples.
  • Document NA-filling of input windows in epi_slide.
  • Fix mix of [.]version[s] and [.]ref_time_value[s] in ?epix_slide. (We might mention you can use .ref_time_value as well as .version inside formulas and data-masking expressions, but we should point to .version first in this case, and to version/.versions in all other discussion.)
  • Make ?epi[x]_slide actually describe what they do. (Starting paragraph maybe isn't really that clear.)
  • [-] Try to remove some unnecessary details/comparison in ?epix_slide
  • Remove link to removed "advanced" vignette.
  • Remove bad epix_slide example
  • [-] Remove overly-complicated / less relevant vignettes.
  • Fix garbled .all_versions docs
  • Fix Details: second bullet point formatting regarding .all_versions
  • [-] Separate out new_* functions into separate documentation pages?
  • Fix misleading/confusing ?epi_df "Slides a given function over variables in an epi_df object.`"
  • Replace median example in ?epi_df with something that's not dominated by epi_slide_opt(frollapply).
  • NEWS: add non-technical main highlights
  • Describe NA behavior noted below.

Thanks @XuedaShen for identifying some of these.
@XuedaShen
Copy link
Contributor

Also a reminder that epi_slide completes the input window with NA, thereby leading to downstream NAs if we don't manually specify na.rm. I wasn't able to find a remark on either complete or NA in the docs of epi_slide.

@brookslogan brookslogan self-assigned this Feb 18, 2025
@brookslogan brookslogan linked a pull request Mar 11, 2025 that will close this issue
Open
4 tasks
@dajmcdon
Copy link
Contributor

dajmcdon commented Apr 8, 2025

  • General practice: all functions should have examples to illustrate at least 1-2 intended use cases.
  • epi_slide_opt() Description field should say "when and why to use this and not epi_slide()" And probably port back to the other. Would be nice to have examples to illustrate the improvement (or cases not handled here).
  • In complete.epi_df() rather than saying "see tidyr::complete" for each argument, it should use #' @inheritParams tidyr::complete. The Description field should say what it does. Then add a link to the tidyr version in the See also field.

@brookslogan
Copy link
Contributor Author

brookslogan commented Apr 8, 2025 •
edited
Loading

I believe we have a lot more "see {x}" than just in complete.epi_df. We can't blindly apply @inheritParams for all of these cases; I think I found some case where ... might have been misleading. Anyway, we just need to read the results of trying @inheritParams and sanity-check / override when necessary.

[One reason we might have used "as in" is to highlight what actually changed compared to some other function. We should probably decide on this tradeoff case by case.]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@brookslogan brookslogan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Doc cleanup cmu-delphi/epiprocess
3 participants
@dajmcdon @brookslogan @XuedaShen