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

Made changes in assign_job_queue.R, cleanup.R, lineage.R, msa.R, tree.R #106

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

Conversation

SunSummoner
Copy link
Collaborator

@SunSummoner SunSummoner commented Oct 20, 2024

Description

What kind of change(s) are included?

  • Feature (adds or updates new capabilities)
  • Bug fix (fixes an issue).
  • Enhancement (adds functionality).
  • Breaking change (these changes would cause existing functionality to not work as expected).

Checklist

Please ensure that all boxes are checked before indicating that this pull request is ready for review.

  • I have read and followed the CONTRIBUTING.md guidelines.
  • I have searched for existing content to ensure this is not a duplicate.
  • I have performed a self-review of these additions (including spelling, grammar, and related).
  • I have added comments to my code to help provide understanding.
  • I have added a test which covers the code changes found within this PR.
  • I have deleted all non-relevant text in this pull request template.
  • Reviewer assignment: @the-mayer and @jananiravi

@the-mayer I have added a globals.R for Protein in Lineage.R As suggested. The names of most of the functions have been changed. I can update those in the issue if you'd want that.

[x] generate_fa2tre: function not found but I add .data$ to ‘tre_path’ in the updated function
[x] get_proc_medians : : no visible global function definition for ‘median’
[x] generate_msa: no visible global function definition for ‘kalign’
[ ] getJobsWithStates: not found
[x] get_proc_medians : : no visible global function definition for ‘median’
[x] IPG2Lineage updated from ipg2lin
[x] ipg2lin: no visible binding for global variable ‘Protein’
[x] ipg2lin: no visible binding for global variable ‘mergedTax’
[x] ipg2lin: no visible binding for global variable ‘gca_ipg_dt’
[x] ipg2lin: no visible binding for global variable ‘assembly_path’
[x] ipg2lin: no visible binding for global variable ‘Lineage’
[x] ipr2viz: no visible binding for global variable ‘iprscan_cols’
[x] ipr2viz: no visible binding for global variable ‘Name’
[x] ipr2viz: no visible binding for global variable ‘Lineage_short’
[x] ipr2viz: no visible binding for global variable ‘QueryName’
[x] ipr2viz: no visible binding for global variable ‘PcPositive’
[x] ipr2viz: no visible binding for global variable ‘AccNum’.
I finished these variables ipr2viz in my previous PR which were approved but even though the PR wasn't merged they have been updated.
[x] write_proc_medians_table: no visible binding for global variable ‘median_seconds’

@jananiravi jananiravi requested a review from the-mayer October 20, 2024 19:39
@jananiravi jananiravi added outreachy for outreachy interns documentation Improvements or additions to documentation, incl. R docstring/roxygen2 labels Oct 20, 2024
R/msa.R Outdated
@@ -196,7 +196,7 @@ createMSA_PDF <- function(fasta_path, out_path = NULL,
#' will be saved.
#'
#' @importFrom Biostrings readAAStringSet
#'
#' @importFrom stats kalign
Copy link
Collaborator

Choose a reason for hiding this comment

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

The stats package does not export kalign(), but looking at the nearby code, we may need to incorporate some code from here: https://github.com/mhahsler/rMSA/blob/master/R/kalign.R

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is likely outside the scope of this PR/issue, but a great find!!

R/tree.R Outdated
@@ -57,10 +57,10 @@
#' here("src/FastTree")
#' }
convertFA2Tree <- function(fa_path = here("data/alns/pspa_snf7.fa"),
tre_path = here("data/alns/pspa_snf7.tre"),
.data$tre_path = here("data/alns/pspa_snf7.tre"),
Copy link
Collaborator

@the-mayer the-mayer Oct 30, 2024

Choose a reason for hiding this comment

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

This .data is not needed.

@@ -57,11 +57,8 @@
#' here("src/FastTree")
#' }
convertFA2Tree <- function(fa_path = here("data/alns/pspa_snf7.fa"),
Copy link
Member

Choose a reason for hiding this comment

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

If these default/example files don't exist, we could remove them. Otherwise, placeholders that work might be a good idea!

R/lineage.R Outdated
@@ -395,10 +395,10 @@ IPG2Lineage <- function(accessions, ipg_file,
{
# browser()
acc <- accessions[i]
acc_inds <- which(mergedTax$Protein == acc)
acc_inds <- which(.data$mergedTax$Protein == acc)
Copy link
Collaborator

@the-mayer the-mayer Oct 31, 2024

Choose a reason for hiding this comment

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

This is not needed and makes me think that the IPG2Lineage() function defined in acc2lin.R may be more recent/relevant. mergedTax looks like it may have been pulled from the Global Environment at some point in the past -- it will not work as it exists in this function.

Again, this is outside the scope of this issue, but something we will need to disambiguate (acc2lin.R vs lineage.R)/correct moving forward!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation, incl. R docstring/roxygen2 outreachy for outreachy interns
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants