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

SNaQ 2.0 #215

Closed
wants to merge 52 commits into from
Closed

SNaQ 2.0 #215

wants to merge 52 commits into from

Conversation

NathanKolbow
Copy link
Contributor

Additions to the SNaQ algorithm:

  • multithreading of pseudolikelihood calculations and all quartet operations
  • new snaq! argument propQuartets: proportion of quartets in the input data to snaq! that are used in the analysis
  • new snaq! argument probQR: uses quartet information to inform the heuristic search of network space

All tests have been updated to fit the updated functions and are passing.

Copy link

codecov bot commented Sep 10, 2024

Codecov Report

Attention: Patch coverage is 37.01299% with 291 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/readData.jl 0.58% 169 Missing ⚠️
src/moves_snaq.jl 63.82% 34 Missing ⚠️
src/addHybrid_snaq.jl 57.89% 32 Missing ⚠️
src/update.jl 0.00% 32 Missing ⚠️
src/snaq_optimization.jl 52.94% 16 Missing ⚠️
src/auxiliary.jl 0.00% 5 Missing ⚠️
src/parsimony.jl 50.00% 1 Missing ⚠️
src/pseudolik.jl 97.82% 1 Missing ⚠️
src/types.jl 66.66% 1 Missing ⚠️
Files with missing lines Coverage Δ
src/PhyloNetworks.jl 100.00% <ø> (ø)
src/bootstrap.jl 70.26% <ø> (ø)
src/parsimony.jl 88.75% <50.00%> (-0.14%) ⬇️
src/pseudolik.jl 81.69% <97.82%> (-0.27%) ⬇️
src/types.jl 83.95% <66.66%> (+1.23%) ⬆️
src/auxiliary.jl 91.20% <0.00%> (-0.51%) ⬇️
src/snaq_optimization.jl 74.79% <52.94%> (-0.40%) ⬇️
src/addHybrid_snaq.jl 69.63% <57.89%> (-4.62%) ⬇️
src/update.jl 81.95% <0.00%> (-11.21%) ⬇️
src/moves_snaq.jl 77.98% <63.82%> (-1.77%) ⬇️
... and 1 more

... and 1 file with indirect coverage changes

@cecileane
Copy link
Member

Some comments for now:

  • It looks like there's a number of new lines of code that are not covered by tests (easy to spot from the "files changed" tab). Could the test suite be modified to have higher code coverage, hopefully without making the tests take even longer to run?
  • Could the new code adhere more to the standard julian style? For example, there should not be parentheses after if, e.g. prefer this:
    if probQR>0.0 && rand() < probQR
    over this:
    if(probQR>0.0 && rand() < probQR)
    This example comes from the julia style guide. The Blue style complements it.

@cecileane
Copy link
Member

@crsl4 and @NathanKolbow : the plan for PhyloNetworks v0.17.0 is to be a "small" core package without any SNaQ-related function, and those functions moved to a new SNaQ.jl package. Instead of this pull request, could you:

  1. start this package, have a version that mirrors the current "SNaQ 1"
  2. and then make a pull request with all these new features for SNaQ 2?

I would be happy to help. For example, I could start a skeleton for SNaQ.jl.

This process was already started to move trait-related functions away from PhyloNetworks: see branch dev01 of PhyloTraits, and branch dev of PhyloNetworks.

@crsl4
Copy link
Member

crsl4 commented Sep 13, 2024

Yes, I agree! I don't think we should merge this in PhyloNetworks right now.

@jjustison
Copy link
Member

I made a skeleton repo here https://github.com/JuliaPhylo/SNaQ.jll for SNaQ. I am not as familiar with SNaQ's code I'd be happy to help further with porting things over to this repo and setting up things like tests.

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.

5 participants