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

Major updates #34

Merged
merged 11 commits into from
Mar 15, 2024
Merged

Major updates #34

merged 11 commits into from
Mar 15, 2024

Conversation

Copy link
Contributor

@rdmorin rdmorin left a comment

Choose a reason for hiding this comment

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

This is a really important set of improvements and serves as a great reminder for where documentation needs to still be filled in. Admittedly, mostly functions I've started and left for the time being.

#' @param bin_df Data frame with bins.
#' @param min_value Minimum value to retain the bin.
#' @param filename Path to a local file on drive to save the resulting file.
#'
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should say "nothing" for the return or (better yet) make functions like this one return the "full path to the file that was written"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, added this to this function and other similar functions that were writing files

R/cache_output.R Outdated
#' @param function_params TODO.
#' @param additional_details TODO.
#'
#' @return file
Copy link
Contributor

Choose a reason for hiding this comment

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

see earlier comment regarding what to return

@@ -31,7 +31,7 @@
#' maf1,
#' regions_bed = grch37_ashm_regions
#' )
#' #' calculate_tmb(
#' calculate_tmb(
Copy link
Contributor

Choose a reason for hiding this comment

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

The title of this function should probably be "calculate tumour mutation burden", to be more intuitive and self-explanatory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks - Updated the title!

R/copy_no_clobber.R Show resolved Hide resolved
@Kdreval Kdreval merged commit 9bcc4fe into master Mar 15, 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
2 participants