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

Autoplot permustats #37

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jarioksa
Copy link
Collaborator

@jarioksa jarioksa commented Jul 8, 2023

This PR provides autoplot and fortify methods for vegan::permustats results. These will eventually replace lattice methods densityplot and qqmath, and provide an alternative ggplot2 graphics for boxplot.

@jarioksa jarioksa requested a review from gavinsimpson July 8, 2023 12:47
Copy link
Owner

@gavinsimpson gavinsimpson left a comment

Choose a reason for hiding this comment

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

Just some general points on

  1. how the tidyverse devs now want one to refer to variables in aes() via the .data mechanism (I need to revisit all my functions too)
  2. lower case names for variables (I need to revisit this in all the old code too - yes, I am going to break things but ggvegan is alpha at best currently)
  3. Return tibbles from fortify() methods
  4. Use markdown instead of Rd where allowed (not all Rd markup has a counterpart to markdown, but the most used Rd markup does)

Feel free to make these changes yourself or I can merge and make the same set of changes as per the comments.

switch(plot,
"box" =,
"violin" = ggplot(df,
aes_(~Term, ~Permutations, fill=~Term,
Copy link
Owner

Choose a reason for hiding this comment

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

aes_ and aes_string are soft-deprecated as of ggplot2 v 3.0.0 (meaning they still work but a warning will be emitted at some time interval which will cause problems with CRAN) Instead use the .data[[var]] idiom, so this should be:

aes(x = .data[[term]], y = .data[[permutations]], fill = .data[[term]],
    colour = .data[[term]])

(Note lower case names)

"violin" = ggplot(df,
aes_(~Term, ~Permutations, fill=~Term,
colour=~Term)),
"density" = ggplot(df, aes_(~Permutations, fill = ~Term,
Copy link
Owner

Choose a reason for hiding this comment

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

aes(y = .data[[permutations]], fill = .data[[term]],
    colour = .data[[term]])

colour=~Term)),
"density" = ggplot(df, aes_(~Permutations, fill = ~Term,
colour=~Term)),
"qqnorm" = ggplot(df, aes_(sample = ~Permutations, colour=~Term)))
Copy link
Owner

Choose a reason for hiding this comment

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

aes(sample = .data[[permutations]], colour = .data[[term]])

#' autoplot(pstat, "qqnorm", facet = TRUE) + geom_qq_line()
#'

#' @importFrom ggplot2 fortify ggplot aes_ geom_hline geom_vline geom_boxplot
Copy link
Owner

Choose a reason for hiding this comment

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

Will need to import .data from package dplyr (see below) and tibble::tibble (see below too) and can delete aes_

#' @importFrom dplyr .data
#' @importFrom tibble tibble

I think that means we'll need to modify DESCRIPTION also to add tibble as an imports pkg.

Comment on lines +95 to +96
data.frame("Permutations" = as.vector(x),
"Term" = factor(rep(lab, each = nrow(x)), levels=lab))
Copy link
Owner

Choose a reason for hiding this comment

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

Using uppercase variables names was a mistake. As was not realising that tibbles are awesome and should be used over data frames esp when working in the tidyverse.

tibble("permutations" = as.vector(x),
      "term" = factor(rep(lab, each = nrow(x)), levels = lab))

Comment on lines +23 to +25
#' \code{\link[ggplot2]{geom_boxplot}},\code{\link[ggplot2]{geom_violin}},
#' \code{\link[ggplot2]{geom_density}} or
#' \code{\link[ggplot2]{geom_qq}}
Copy link
Owner

Choose a reason for hiding this comment

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

Replace with

#'     [ggplot2::geom_boxplot()], [ggplot2::geom_violin()],
#'     [ggplot2::geom_density()] or [ggplot2::geom_qq()]

#' build diagnostic plots. \code{autoplot} provides quick basic graphs
#' with limited flexibility.
#'
#' @param object Result object from \code{\link[vegan]{permustats}}.
Copy link
Owner

Choose a reason for hiding this comment

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

Change the Rd markup to markdown equivalent

[vegan::permustats()]

Comment on lines +10 to +14
#' Function \code{fortify} returns a data frame with variables
#' \code{Permutations} (numeric) and \code{Term} (factor labelling the
#' permutation). The result of \code{fortify} can be used to custom
#' build diagnostic plots. \code{autoplot} provides quick basic graphs
#' with limited flexibility.
Copy link
Owner

Choose a reason for hiding this comment

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

markdownify:

#' Function `fortify` returns a tibble (data frame) with variables
#' `permutations` (numeric) and `term` (factor labelling the
#' permutation). The result of `fortify()` can be used to custom
#' build diagnostic plots. [autoplot.permutatst()] provides basic graphs
#' with limited flexibility.

Comment on lines +5 to +8
#' Alternatives for \pkg{lattice} graphics functions
#' \code{\link[vegan]{densityplot.permustats}},
#' \code{\link[vegan]{qqmath.permustats}} and
#' \code{\link[vegan]{boxplot.permustats}}.
Copy link
Owner

Choose a reason for hiding this comment

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

markdownify

#' Alternatives for \pkg{lattice} graphics functions
#' [vegan::densityplot.permustats()], [vegan::qqmath.permustats()] and
#' [vegan::boxplot.permustats()].

@jarioksa
Copy link
Collaborator Author

jarioksa commented Jul 12, 2023

Gav,

Feel free to make those fixes. I'll try follow the lead for the next pull requests.

There is one thing that I do not understand: why do you think that using Title case is bad for variables? After all, plot labels usually are title case, and it is natural to have them title case also in fortified scores. Even if you have some fancy package that does magic on names, doing it explicitly and plainly is much sounder imho. These fortified scores are not real data files, but messy interim result mixing data and formatting passed to plotting functions, and users may want to use them for their extra purposes – and may not have magic.

PS. vegan::scores methods have now argument tidy = TRUE which return a data.frame for tidy. I think most of these use title case for variables. In ordiggplot I still used ggvegan::fortify, but vegan::scores might be better as it can return all scores with arg display="all" (incl. regression scores). However, it is sure I won't import tibbles to vegan but they will always be data frames.

@gavinsimpson
Copy link
Owner

There is one thing that I do not understand: why do you think that using Title case is bad for variables? After all, plot labels usually are title case, and it is natural to have them title case also in fortified scores.

This is mainly a stylistic thing, but I also find using lowercase column names easier to code with - I don't have to remember how I capitalised things if I simply don't do it

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