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

ggplot2 plot_mvgam_resids() #84

Closed
wants to merge 4 commits into from

Conversation

mhollanders
Copy link

No description provided.

@nicholasjclark
Copy link
Owner

nicholasjclark commented Oct 27, 2024

Thanks @mhollanders, this is a great start. There are just a few other edits I'd recommend.

  1. Remove calls to library (i.e. there should not be any mentions of library(ggplot2) or library(patchwork))
  2. Add the relevant package with scope resolution when using any ggplot2 or patchwork functions (i.e. use ggplot2::geom_hline rather than geom_hline), as this negates the need for me to specifically import those functions from the package namespaces
  3. Add patchwork, with a relevant minimum version number, to the DESCRIPTION under Imports (https://github.com/nicholasjclark/mvgam/blob/master/DESCRIPTION)

Copy link
Owner

@nicholasjclark nicholasjclark left a comment

Choose a reason for hiding this comment

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

  • We need to add ggdist to Imports if it is necessary to use stat_lineribbon and median_qi. Or we think of other ways to show the same output without adding another dependency
  • Edit the descriptions in each function's @return slot (in the roxygen comments at the top of each function) to state that the expected return is a ggplot object
  • Need to add a line at the top of NEWS to briefly describe these changes
  • Look through tests in tests/test-example-processing.R and change any expectations for return types of these plots (there is an expect_ggplot() function that you can use)
  • Add 'lag' and 'resids' to globalVariables in globals.R

@nicholasjclark
Copy link
Owner

All merged now. Thanks so much @mhollanders

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.

2 participants