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

Add prettify to validate_indices #59

Merged
merged 5 commits into from
Jan 11, 2024

Conversation

WackerO
Copy link
Collaborator

@WackerO WackerO commented Jan 5, 2024

ATM, a call to prettifyVariablename is missing in validate_indices while stringsToNamedVector contains a prettify. This leads to exploratory_plots.R throwing an error Invalid assays because it will e.g. compare the strings raw and Raw. This PR adds the prettify to fix this issue.

@WackerO WackerO requested a review from pinin4fjords January 5, 2024 12:48
R/accessory.R Outdated
@@ -1271,6 +1271,7 @@ validate_indices <- function(assay_data, index_string) {
indices <- as.integer(simpleSplit(index_string))
} else {
indices <- simpleSplit(index_string)
indices <- lapply(indices, prettifyVariablename)
Copy link
Owner

Choose a reason for hiding this comment

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

You'll need an unlist here to maintain the same type as simpleSplit.

@WackerO WackerO requested a review from pinin4fjords January 9, 2024 07:55
Copy link
Owner

@pinin4fjords pinin4fjords left a comment

Choose a reason for hiding this comment

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

Sorry, just looking at this properly.

You'll notice that stringsToNamedVector has the prettification as a parameter. It should probably be the same in this function (with the same default), so that things can be matched.

@WackerO
Copy link
Collaborator Author

WackerO commented Jan 11, 2024

Sorry, just looking at this properly.

You'll notice that stringsToNamedVector has the prettification as a parameter. It should probably be the same in this function (with the same default), so that things can be matched.

Ah yes sure, done!

Copy link
Owner

@pinin4fjords pinin4fjords left a comment

Choose a reason for hiding this comment

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

LGTM

@WackerO WackerO merged commit 919965d into pinin4fjords:develop Jan 11, 2024
1 check passed
@WackerO WackerO deleted the explo_fix_prettify branch January 11, 2024 09:58
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