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

deprecate Taxa names -> Features #91

Merged
merged 25 commits into from
Jan 29, 2024
Merged

Conversation

himmil
Copy link
Contributor

@himmil himmil commented Sep 7, 2023

No description provided.

@himmil
Copy link
Contributor Author

himmil commented Sep 7, 2023

@TuomasBorman can you merge?

@TuomasBorman
Copy link
Contributor

Tests (tests/testthat directory) seems still have deprecated functions

@TuomasBorman
Copy link
Contributor

TuomasBorman commented Oct 17, 2023

Related to what we discussed today; to replace aes_string:

There are two cases where we have used aes_string: "the standard way", i.e. case 1, and with do.call function, i.e., case 3

Below you will find, how to replace these

library(ggplot2)

data(cars)


######################### CASE 1
p <- ggplot(cars)

p + geom_point(aes(x = speed, y = dist))

p + geom_point(aes_string(x = "speed", y = "dist"))

p + geom_point(aes(x = .data[["speed"]], y = .data[["dist"]]))

######################### CASE2

p + aes(x = speed, y = dist) + geom_point()

p + aes_string(x = "speed", y = "dist") + geom_point()

p + aes(x = .data[["speed"]], y = .data[["dist"]]) + geom_point()

############################ CASE 2.1

aesthetic <- aes(x = speed, y = dist)
p + aesthetic + geom_point()

aesthetic <- aes_string(x = "speed", y = "dist")
p + aesthetic + geom_point()


aesthetic <- aes(x = .data[["speed"]], y = .data[["dist"]])
p + aesthetic + geom_point()

################# CASE 3

# IS not working
args <- list(x = "speed", y = "dist")
aesthetic <- do.call(aes, args)
p + aesthetic + geom_point()

# Works
args <- list(x = "speed", y = "dist")
aesthetic <- do.call(aes_string, args)
p + aesthetic + geom_point()

# Option 1 (I think this is better)
args <- list(x = "speed", y = "dist")
args <- lapply(args, function(x) if (!is.null(x)) sym(x))
aesthetic <- do.call(aes, args)
p + aesthetic + geom_point()

# Option 2
args <- list(x = "speed", y = "dist")
args <- lapply(args, function(x) if (!is.null(x)) sym(x))
aesthetic <- aes(!!!args)
p + aesthetic + geom_point()

R/plotColTile.R Outdated Show resolved Hide resolved
R/plotColTile.R Outdated Show resolved Hide resolved
R/plotColTile.R Outdated Show resolved Hide resolved
R/utils_plotting.R Outdated Show resolved Hide resolved
@TuomasBorman
Copy link
Contributor

Error occurs when the data is loaded

no visible global function --> add importFrom tags (check other functions for example)

R/utils_plotting.R Outdated Show resolved Hide resolved
@himmil
Copy link
Contributor Author

himmil commented Oct 19, 2023

Now, finally, it should be ready to be merged.

@TuomasBorman
Copy link
Contributor

Sorry for late reply. Can you run BiocCheck::BiocCheck(). It seems that the indentation is off (should be multiplication of 4 spaces)

Copy link
Contributor

@TuomasBorman TuomasBorman left a comment

Choose a reason for hiding this comment

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

After indentation check, this is good to go

@himmil
Copy link
Contributor Author

himmil commented Jan 19, 2024

Hi, I ran the check and it gave an error that installation calls found in vignettes. How should I fix it?

@TuomasBorman
Copy link
Contributor

You can skip those errors not directly related to this PR. We can fix those things in other PRs

@himmil
Copy link
Contributor Author

himmil commented Jan 29, 2024

Ok, this is ready to be merged (I don't have write access to do it).

@TuomasBorman TuomasBorman merged commit adc8e9e into microbiome:master Jan 29, 2024
1 check passed
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