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

Plot function #8

Merged
merged 47 commits into from
May 29, 2024
Merged

Plot function #8

merged 47 commits into from
May 29, 2024

Conversation

CarlosPoses
Copy link
Contributor

Hi @thomvolker , here the first draft of the plot function. A lot of features to add and discuss yet.

CarlosPoses and others added 30 commits September 12, 2023 10:55
Separetes plot functions into three functions
1. Default: Plots density ratio histograms of both samples
2. Univariate: Plot density ratio value against value of one variable.
3. Bivariate: Plot density ratio value (in colors), for combinations of two variables
implemented several changes in plot functions (separe objects, simply code, use base pipe, use default bins, separate bins visually, add argument to plot x with logscale, add argument for shapes)
checks in all plots for object type, names not overriden, names in original dataset
- Bivariate plot fuctions plots bivariate plots of chosen variables
- Adjust some aesthethics (fill, colour, alpha, shape) to improve visibility
- Add roxigeny2 to include documentation (+export)
Solve a couple of small bugs,added informative messages, added histogram binwidth as an argument
Remove duplicate variables, add heatmap
changed histogram function, change warning called, changed value transformation
Fix small bug related to variable names in variable grid. Make both bivariates plot show logarithm(density ratio) by default, and ajust scales based on it
small bug fix with heatmap related to negative predicted density ratios
added bins argument for histogram; made logarithmic transformation warning conditional on negative values present, mapped colours to sample in univariate plot
improved warning messages; make bivariate plot only show the lower diagonal
minor edit in colours bivariate heatmap
Solve small bug warning, rearrageged numerator and denominator labels univariate plot, create assembled outplot in univariate plots, implemented "smart" labels on y axis
fix labels, add possibility of choosing number of rows univariate plot
add show.sample argument for bivariate plots, add printing method
R/print.R Outdated Show resolved Hide resolved
R/plot.R Outdated Show resolved Hide resolved
R/plot.R Outdated
#'
#' Plots a scatterplot of two variables, with densityratio mapped to the colour scale.
#'
#' @inheritParams plot.univariate
Copy link
Owner

Choose a reason for hiding this comment

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

Try with space inbetween

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It did not work. The solution suggested here (https://stackoverflow.com/questions/47782004/how-to-use-inheritparams-on-single-parameters-when-multiple-parameters-match) involves creating additional files, which I think is more cumbersome than helpful for a single repeated parameter. So I just solved it by defining the repeated parameter twice.

R/checks.R Outdated Show resolved Hide resolved
.gitignore Outdated
@@ -2,3 +2,4 @@
.Rhistory
.RData
.Ruserdata
\playground
Copy link
Owner

Choose a reason for hiding this comment

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

Don't forget to remove the playground folder again (and also remove it from the .gitignore then).

R/plot.R Outdated Show resolved Hide resolved
R/plot.R Outdated Show resolved Hide resolved
R/plot.R Outdated Show resolved Hide resolved
R/plot.R Outdated Show resolved Hide resolved
Copy link
Owner

@thomvolker thomvolker left a comment

Choose a reason for hiding this comment

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

Code looks really good! I have made some suggestions below (removing some dependencies and make some code a bit neater, but overall it was really good already). Let me know if you have any questions.

R/plot.R Outdated Show resolved Hide resolved
R/plot.R Outdated Show resolved Hide resolved
R/plot.R Outdated Show resolved Hide resolved
R/plot.R Outdated Show resolved Hide resolved
R/plot.R Outdated Show resolved Hide resolved
R/plot.R Outdated Show resolved Hide resolved
R/plot.R Outdated Show resolved Hide resolved
R/plot.R Outdated Show resolved Hide resolved
R/plot.R Outdated
#'
#' @examples
plot_bivariate <- function(object, vars, samples = "both",
output = "assembled", logscale = TRUE, show.sample = FALSE) {
Copy link
Owner

Choose a reason for hiding this comment

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

Can we also have vars1, vars2 = NULL, and if vars2 is specified, it must be of the same length as vars1, and specifies what each variable in vars1 must be plotted against (this can be nice if you have many variables and only want to look at some combinations). Then we can simply have (later on)

if (is.null(vars2)) {
var_combinations = expand.grid(vars1, vars1)
} else {
var_combinations = matrix(c(vars1, vars2), ncol = 2)
} 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did that! I also added one check so that if the length of vars1 == 1 and vars2 == 1, then it defaults to grid = FALSE (A grid makes no sense for one plot). And another one so that if length(vars1) and &is.nul(vars2), then there's an error (youneed to variables for a bivariate plot).

R/plot.R Outdated

# Erase upper diagonal
## Create plot into a grob
grob <- ggplotGrob(plot)
Copy link
Owner

Choose a reason for hiding this comment

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

If we add the vars2 = NULL argument, we also need to adjust this code here, such that it is only executed if is.null(vars2).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. Var2 and Var1 are the defaults names created when you do expand.grid (maybe that's where the confusion comes from?). This code is just removing the upper diagonal. It is working right now regardless of whether you specify var2 or not.

Solve final issues before CRAN submission.
Concretely:
- solve small typos/improve comments
- creates function check.logscale()
- adds plot.lhss and plot.spectral
- minor changes to legiility of code
- adds check.var.combinations function
solve bug in predict function (newdata != data)
@thomvolker thomvolker merged commit 6620962 into thomvolker:main May 29, 2024
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