Fixes Issue #94 - #108

Klangina · 2024-10-22T15:30:56Z

Refactored and merged R/tree.R CreateFA2Tree and ConvertFA2Tree.
Parametrized every hardcoded value and path in ConvertFA2Tree
Refactored convertAlignment2Trees to support running both modes.
Backward compatible
Updated all documentation for R/tree.R

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: Tag a relevant team member to review and approve the changes.

Klangina · 2024-10-22T15:38:48Z

@jananiravi I have made the changes. Kindly review/guide me for further steps if required in this Issue.

- Refactored and merged R/tree.R CreateFA2Tree and ConvertFA2Tree. - Parametrized every hardcoded value and path in ConvertFA2Tree - Refactored convertAlignment2Trees to support running both modes. - Backward compatible - Updated all documentation for R/tree.R

Klangina · 2024-10-22T15:51:24Z

FIxed the commit message to reflect the correct issue number

epbrenner

A couple thoughts and suggestions as I passed through. I'm not 100% certain on how the tree generation is meant to flow from the older script you're refactoring, so please ask me for clarification or feel free to just toss suggestions if they don't align with the actual logic going on here!

epbrenner · 2024-10-22T17:15:27Z

R/tree.R

-    # estimate a distance matrix using a Jules-Cantor Model
-    dna_dist <- dist.ml(prot10, model = "JC69")
+        # estimate a distance matrix using a Jules-Cantor Model
+        dna_dist <- dist.ml(prot_subset, model = dna_model)


Suggested change

dna_dist <- dist.ml(prot_subset, model = dna_model)

prot_dist <- dist.ml(prot_subset, model = mt)

I know the original outdated code references DNA here, but I believe this should all be protein based on the inputs. If the change to prot_dist is correct, the code below will also need modification to reflect the variable name changes.

@jananiravi : Can you confirm if the above can be considered correct?

@epbrenner are these the unused tree generators? Using dna could be a stub? Maybe change to protein for now (will work with current molevolvr) or make it a function argument to take dna or protein fasta sequences -- future-proofing the function given starting with DNA seqs will be part of molevolvr sooner rather than later. Does that answer your question, @Klangina ?

Sure @jananiravi, I will create an argument to switch between DNA and protein sequences and keep the default as protein. Will update the pr shortly.

epbrenner · 2024-10-22T17:15:49Z

R/tree.R

-    plot(prot_NJ, main = "Neighbor Joining")
+        ## Neighbor Joining, UPGMA, and Maximum Parsimony
+        # quick and dirty UPGMA and NJ trees
+        prot_UPGMA <- upgma(dna_dist)


Suggested change

prot_UPGMA <- upgma(dna_dist)

prot_UPGMA <- upgma(prot_dist)

epbrenner · 2024-10-22T17:15:59Z

R/tree.R

+        ## Neighbor Joining, UPGMA, and Maximum Parsimony
+        # quick and dirty UPGMA and NJ trees
+        prot_UPGMA <- upgma(dna_dist)
+        prot_NJ <- NJ(dna_dist)


Suggested change

prot_NJ <- NJ(dna_dist)

prot_NJ <- NJ(prot_dist)

epbrenner · 2024-10-22T17:21:07Z

R/tree.R

+        # ml estimation w/ distance matrix
+        fit <- pml(prot_NJ, prot_subset)
+        print(fit)
+        fitJC <- optim.pml(fit, model = ml_model, rearrangement = rearrangement)


Suggested change

fitJC <- optim.pml(fit, model = ml_model, rearrangement = rearrangement)

fit_optimized <- optim.pml(fit, model = fit, rearrangement = rearrangement)

Or something similar. JC here stands for Jukes-Cantor, which is a DNA substitution model, so removing that from the variable. I believe the model in model = ml_model will actually be the prior tree generated by pml(), which would be fit here, and then optim.pml() optimizes the previous model. Do double-check me on this, though!

Yes, switching model based on DNA/protein input could be part of the function, too.

epbrenner · 2024-10-22T17:21:38Z

R/tree.R

+        fit <- pml(prot_NJ, prot_subset)
+        print(fit)
+        fitJC <- optim.pml(fit, model = ml_model, rearrangement = rearrangement)
+        logLik(fitJC)


Suggested change

logLik(fitJC)

logLik(fit_optimized)

If above suggestion is correct.

epbrenner · 2024-10-22T17:21:59Z

R/tree.R

+            bs = bootstrap_reps, optNni = TRUE, multicore = TRUE,
+            control = pml.control(trace = 0)
+        )
+        plotBS(midpoint(fitJC$tree), bs, p = bootstrap_p, type = plot_type)


Suggested change

plotBS(midpoint(fitJC$tree), bs, p = bootstrap_p, type = plot_type)

plotBS(midpoint(fit_optimized$tree), bs, p = bootstrap_p, type = plot_type)

epbrenner · 2024-10-22T17:22:50Z

R/tree.R

+        prot_subset_NJ <- NJ(prot_subset_dm)
+        fit2 <- pml(prot_subset_NJ, data = prot_subset)
+        print(fit2)
+        fitJC2 <- optim.pml(fit2, model = ml_model, rearrangement = rearrangement)


Suggested change

fitJC2 <- optim.pml(fit2, model = ml_model, rearrangement = rearrangement)

fit_optimized2 <- optim.pml(fit2, model = fit2, rearrangement = rearrangement)

As before, if model should be the output of pml() then update. Discard if I'm mistaken though!

epbrenner · 2024-10-22T17:23:00Z

R/tree.R

+        fit2 <- pml(prot_subset_NJ, data = prot_subset)
+        print(fit2)
+        fitJC2 <- optim.pml(fit2, model = ml_model, rearrangement = rearrangement)
+        logLik(fitJC2)


Suggested change

logLik(fitJC2)

logLik(fit_optimized2)

epbrenner · 2024-10-22T17:23:09Z

R/tree.R

+        print(fit2)
+        fitJC2 <- optim.pml(fit2, model = ml_model, rearrangement = rearrangement)
+        logLik(fitJC2)
+        bs_subset <- bootstrap.pml(fitJC2,


Suggested change

bs_subset <- bootstrap.pml(fitJC2,

bs_subset <- bootstrap.pml(fit_optimized2,

epbrenner · 2024-10-22T17:23:18Z

R/tree.R

+            bs = bootstrap_reps, optNni = TRUE, multicore = TRUE,
+            control = pml.control(trace = 0)
+        )
+        bs2 <- plotBS(midpoint(fitJC2$tree), bs, p = bootstrap_p, type = plot_type)


Suggested change

bs2 <- plotBS(midpoint(fitJC2$tree), bs, p = bootstrap_p, type = plot_type)

bs2 <- plotBS(midpoint(fit_optimized2$tree), bs, p = bootstrap_p, type = plot_type)

- renamed dna_dist to seq_dist - renamed fitJC and fitJC2 to fit_optimised and fit_optimised2 to make variable names more general. - Updated code and documentation accordingly.

Klangina · 2024-10-23T13:27:33Z

@epbrenner @jananiravi : Made changes as per recommendations before. Kindly review.

… following two issues: createFA2Tree: no visible global function definition for ‘midpoint’ createFA2Tree: no visible binding for global variable ‘fit_optimized2’

Fixes Issue JRaviLab#94-

4124b68

- Refactored and merged R/tree.R CreateFA2Tree and ConvertFA2Tree. - Parametrized every hardcoded value and path in ConvertFA2Tree - Refactored convertAlignment2Trees to support running both modes. - Backward compatible - Updated all documentation for R/tree.R

Klangina force-pushed the Issue-94 branch from 79cf7e3 to 4124b68 Compare October 22, 2024 15:49

Klangina changed the title ~~Fixes Issue #90 -~~ Fixes Issue #94 - Oct 22, 2024

epbrenner reviewed Oct 22, 2024

View reviewed changes

- Created modes to switch between DNA and protien sequences.

dd23e00

- renamed dna_dist to seq_dist - renamed fitJC and fitJC2 to fit_optimised and fit_optimised2 to make variable names more general. - Updated code and documentation accordingly.

Klangina mentioned this pull request Oct 28, 2024

R-CMD Check: no visible binding for global variable #4

Closed

Fixed two typos and added explicit import for midpoint to resolve the…

3ed285b

… following two issues: createFA2Tree: no visible global function definition for ‘midpoint’ createFA2Tree: no visible binding for global variable ‘fit_optimized2’

Klangina mentioned this pull request Oct 28, 2024

Tree.R : R-CMD check #113

Open

5 tasks

Changes for fixing Issue-JRaviLab#113

33e0f1f

	dna_dist <- dist.ml(prot_subset, model = dna_model)
	prot_dist <- dist.ml(prot_subset, model = mt)

	fitJC <- optim.pml(fit, model = ml_model, rearrangement = rearrangement)
	fit_optimized <- optim.pml(fit, model = fit, rearrangement = rearrangement)

	plotBS(midpoint(fitJC$tree), bs, p = bootstrap_p, type = plot_type)
	plotBS(midpoint(fit_optimized$tree), bs, p = bootstrap_p, type = plot_type)

	fitJC2 <- optim.pml(fit2, model = ml_model, rearrangement = rearrangement)
	fit_optimized2 <- optim.pml(fit2, model = fit2, rearrangement = rearrangement)

	bs_subset <- bootstrap.pml(fitJC2,
	bs_subset <- bootstrap.pml(fit_optimized2,

	bs2 <- plotBS(midpoint(fitJC2$tree), bs, p = bootstrap_p, type = plot_type)
	bs2 <- plotBS(midpoint(fit_optimized2$tree), bs, p = bootstrap_p, type = plot_type)

Uh oh!

Fixes Issue #94 - #108

Are you sure you want to change the base?

Fixes Issue #94 - #108

Uh oh!

Conversation

Klangina commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

What kind of change(s) are included?

Checklist

Uh oh!

Klangina commented Oct 22, 2024

Uh oh!

Klangina commented Oct 22, 2024

Uh oh!

epbrenner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Klangina commented Oct 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Klangina commented Oct 22, 2024 •

edited

Loading